Compile error on Linux due to new File::write function

[TD-er]

See also this discussion: b1da9ed#r32622483

@devyte asked to open an issue about this.

In pull request #5525 there has been a new function added to the File class:

Arduino/cores/esp8266/FS.h

Lines 82 to 83 in b1da9ed

	// Arduino "class SD" methods for compatibility
	size_t write(const char *str) { return write((const uint8_t*)str, strlen(str)); }

This function causes compiler issues on Linux.

In file included from /home/gijs/GitHub/letscontrolit/ESPEasy/src/ESPEasy.ino:86:0:
/home/gijs/GitHub/letscontrolit/ESPEasy/src/ESPEasyStorage.ino: In function 'String ClearInFile(char*, int, int)':
/home/gijs/GitHub/letscontrolit/ESPEasy/src/ESPEasyStorage.ino:679:30: error: call of overloaded 'write(unsigned int)' is ambiguous
SPIFFS_CHECK(f.write(0), fname);
^

In short, this new function makes it impossible to use the existing function write(uint8_t)

Arduino/cores/esp8266/FS.h

Lines 55 to 56 in b1da9ed

	size_t write(uint8_t) override;
	size_t write(const uint8_t *buf, size_t size) override;

[d-a-v]

@TD-er Can you try with this small patch ? cores/esp8266/FS.h L83

    // Arduino "class SD" methods for compatibility
+   size_t write(int i) { return write((uint8_t)i); } // avoid ambiguity
    size_t write(const char *str) { return write((const uint8_t*)str, strlen(str)); }

[TD-er]

I tested it and as expected it now has even more options to choose from, so it is even more ambigue :)

In file included from /home/gijs/GitHub/letscontrolit/ESPEasy/src/ESPEasy.ino:86:0:
/home/gijs/GitHub/letscontrolit/ESPEasy/src/ESPEasyStorage.ino: In function 'String InitFile(const char*, int)':
/home/gijs/GitHub/letscontrolit/ESPEasy/src/ESPEasyStorage.ino:575:30: error: call of overloaded 'write(unsigned int)' is ambiguous
SPIFFS_CHECK(f.write(0u), fname);
^
src/ESPEasy-Globals.h:2263:43: note: in definition of macro 'SPIFFS_CHECK'
#define SPIFFS_CHECK(result, fname) if (!(result)) { return(FileError(__LINE__, fname)); }
^
/home/gijs/GitHub/letscontrolit/ESPEasy/src/ESPEasyStorage.ino:575:30: note: candidates are:
SPIFFS_CHECK(f.write(0u), fname);
^
src/ESPEasy-Globals.h:2263:43: note: in definition of macro 'SPIFFS_CHECK'
#define SPIFFS_CHECK(result, fname) if (!(result)) { return(FileError(__LINE__, fname)); }
^
In file included from src/ESPEasy-Globals.h:579:0,
from /home/gijs/GitHub/letscontrolit/ESPEasy/src/ESPEasy.ino:86:
/home/gijs/.platformio/packages/framework-arduinoespressif8266@src-31d658a59f41540201fc3726a1394910/cores/esp8266/FS.h:55:12: note: virtual size_t fs::File::write(uint8_t)
size_t write(uint8_t) override;
^
/home/gijs/.platformio/packages/framework-arduinoespressif8266@src-31d658a59f41540201fc3726a1394910/cores/esp8266/FS.h:83:12: note: size_t fs::File::write(int)
size_t write(int i) { return write((uint8_t)i); } // avoid ambiguity
^
/home/gijs/.platformio/packages/framework-arduinoespressif8266@src-31d658a59f41540201fc3726a1394910/cores/esp8266/FS.h:84:12: note: size_t fs::File::write(const char*)
size_t write(const char *str) { return write((const uint8_t*)str, strlen(str)); }
^
In file included from /home/gijs/GitHub/letscontrolit/ESPEasy/src/ESPEasy.ino:86:0:
/home/gijs/GitHub/letscontrolit/ESPEasy/src/ESPEasyStorage.ino: In function 'String ClearInFile(char*, int, int)':
/home/gijs/GitHub/letscontrolit/ESPEasy/src/ESPEasyStorage.ino:679:30: error: call of overloaded 'write(unsigned int)' is ambiguous
SPIFFS_CHECK(f.write(0u), fname);
^
src/ESPEasy-Globals.h:2263:43: note: in definition of macro 'SPIFFS_CHECK'
#define SPIFFS_CHECK(result, fname) if (!(result)) { return(FileError(__LINE__, fname)); }
^
/home/gijs/GitHub/letscontrolit/ESPEasy/src/ESPEasyStorage.ino:679:30: note: candidates are:
SPIFFS_CHECK(f.write(0u), fname);
^
src/ESPEasy-Globals.h:2263:43: note: in definition of macro 'SPIFFS_CHECK'
#define SPIFFS_CHECK(result, fname) if (!(result)) { return(FileError(__LINE__, fname)); }
^
In file included from src/ESPEasy-Globals.h:579:0,
from /home/gijs/GitHub/letscontrolit/ESPEasy/src/ESPEasy.ino:86:
/home/gijs/.platformio/packages/framework-arduinoespressif8266@src-31d658a59f41540201fc3726a1394910/cores/esp8266/FS.h:55:12: note: virtual size_t fs::File::write(uint8_t)
size_t write(uint8_t) override;
^
/home/gijs/.platformio/packages/framework-arduinoespressif8266@src-31d658a59f41540201fc3726a1394910/cores/esp8266/FS.h:83:12: note: size_t fs::File::write(int)
size_t write(int i) { return write((uint8_t)i); } // avoid ambiguity
^
/home/gijs/.platformio/packages/framework-arduinoespressif8266@src-31d658a59f41540201fc3726a1394910/cores/esp8266/FS.h:84:12: note: size_t fs::File::write(const char*)
size_t write(const char *str) { return write((const uint8_t*)str, strlen(str)); }
^

[TD-er]

I'm not sure if it is worth the trouble, but you can use these template things to make the function parameters strongly typed.
See this StackOverflow thread (N.B. I find this answer to be the most clean and readable option suggested in that thread. This one is probably the easiest to implement in this use case)
It does cause other issues, like when you call the function with something that isn't exactly of that type, it will not compile.
e.g. when used on the write(uint8_t) function, if it is called with a char or int, it will not compile.

But I guess it could be used on the newly added write(const char*) function, since there will not be many other types that should be cast to const char*.
On the other hand, is this convenience function really needed? Why not call it write_str(const char*) then?

[Barracuda09]

And please do not use 'old-style' cast in C++

[d-a-v]

@TD-er then add

+   size_t write(unsigned int i) { return write((uint8_t)i); } // avoid ambiguity

@Barracuda09 following your comment I just read this discussion (there are tons like it).
The fact is that I know what C-style cast does and C++ is supposed to be compatible with that.

TBH, I don't see why the above code is wrong. We are dealing with basic scalars here, and we need to tell the compiler how to redirect overloaded calls.
While it could be written in an understandable better C++ way as

+   size_t write(unsigned int i) { return write(uint8_t(i)); } // avoid ambiguity

I don't know which of static_cast<> or dynamic_cast<> should be used. I understand that reinterpret_cast<> is a bit delicate to use and should be wrong here. These keywords are here to render the code ugly (Bjarne said, if I read well the above link) because cast should not be used. How else could we do here ?

Please have a look to IPAddress class, because I had exactly the same issue in it as here, and there are lots of C-style cast and overloads to help the compiler chose the right methods, for the same reason: backward compatibility.

edit:
If the template way is a good solution, please proceed, try it, make a PR :)

[Barracuda09]

@d-a-v

I don't know which of static_cast<> or dynamic_cast<> should be used

That is the whole idea, you have to think about it (and not let the compiler make a choice) but in this case a static_cast would do.

But looking at the code and 'patch' make me think something else is not adding up

[TD-er]

This is enough for me to make it compile:

gijs@Imagine:~/.platformio/packages/framework-arduinoespressif8266@src-31d658a59f41540201fc3726a1394910/cores/esp8266$ git diff
diff --git a/cores/esp8266/FS.h b/cores/esp8266/FS.h
index 07bfb61..75f4959 100644
--- a/cores/esp8266/FS.h
+++ b/cores/esp8266/FS.h
@@ -80,6 +80,11 @@ public:
     bool isDirectory() const;

     // Arduino "class SD" methods for compatibility
+    template<typename T>
+    size_t write(T arg1);
+    size_t write(char i)         { return write(static_cast<uint8_t>(i)); } // avoid ambiguity
+    size_t write(unsigned int i) { return write(static_cast<uint8_t>(i)); } // avoid ambiguity
+    size_t write(int i)          { return write(static_cast<uint8_t>(i)); } // avoid ambiguity
     size_t write(const char *str) { return write((const uint8_t*)str, strlen(str)); }
     void rewindDirectory();
     File openNextFile();
@@ -92,6 +97,7 @@ protected:
     // Arduino SD class emulation
     std::shared_ptr<Dir> _fakeDir;
     FS                  *_baseFS;
+
 };

 class Dir {

[d-a-v]

@TD-er I don't understand your use of template. Is it mandatory for your code to compile ?

@Barracuda09 (or any c++ advisor) is it an acceptable c++ way of casting:

size_t write(unsigned int i) { return write(uint8_t(i)); }

Here we use a constructor, it is not C-way and we can exactly and explicitly understand what it does.
Are *-cast<> preferred over this ? And why ?

[TD-er]

About the casts, when calling it via the constructor, you're creating somewhat of a deepcopy of the data (depending on the copy constructor of the object of course)
With objects like integer types, that's not really an issue, but what if you're using it with more complex objects, like a String or some array type.
Then you have to create a new object, maybe allocate memory (either on stack or heap), etc. This takes resources.
Using a cast (static_cast for example) should be more efficient.
I don't know the exact details of what the compiler does here and some types of casts may lead to extra code generated by the compiler. (like casts that do type checks at runtime and thus may result in returning a nullptr)

But in general the main advantage of using casts is that you're doing it on the existing object.

About the template part. Yep that was needed to instruct the compiler to consider those definitions of size_t write(..) with a single variable to be stricktly typed.
Downside is that you have to define it for every possible type of variable you intend to use.

[Barracuda09]

@TD-er
Thanks for your explanation, and it standsout more, so you can find were casts are done.

[d-a-v]

With objects like integer types, that's not really an issue, but what if you're using it with more complex objects, like a String or some array type.

I can't think char* x; String y = static_cast<String>(x); will do anything else than calling String contructor, which makes it similar to y = String(x); (or y = (String)x;).

I understand static_cast<> and dynamic_cast<> should be used with classes that have inheritance relationship (like A* a = new SonOfA; SonOfA* soa = dynamic_cast<SonOfA*>(a);) but other than that, there's no other option than using the constructor.

About x = reinterpret_cast<y>;, it's the c++ translation of x = *(((typeof(x)*)&y); which is dangerous when one does not know what he is doing.

And I just don't understand the reason of the existence of const_cast<>.

My point is to avoid using magic words when they are obscur. One could tell me that we must know the langage, and that's true. But still, you must always know what you are doing. void f (String p) is very correct and far more harmful than void f (const String& p). So we always must know what we are doing, and my understanding of this discards the use of *_cast<> when we know exactly what we are doing.

About the template part. Yep that was needed to instruct the compiler to consider those definitions of size_t write(..) with a single variable to be stricktly typed.
Downside is that you have to define it for every possible type of variable you intend to use.

I still don't understand. There's no code for it and you later explicitly defines write() with every possible type of variable the core is using.

-- in the quest for coherency and truth :)

edit: @jfbastien there's an occasion to guide the blinds to the light if ever you deign looking to this
(we're c++11)

[TD-er]

A very short reply, since I have to go and pick up my daughter from school...

The const_cast<> is one of the worst named things I guess.
You're removing the const-ness of an object so you can use it as a parameter in a function which hasn't declared const (for a reference parameter) in the arguments, but isn't changing anything in the data itself.
For all other use cases, it is even more tricky than the reinterpret_cast, since that last one is at least showing predictable (and thus reproducible) results when used :)

The other points you mention also deserve a proper reply, but maybe someone else can explain the C++ details better than me (and I have to go now)

[Barracuda09]

@d-a-v consider this code

const char *c = "Hello World";
char * p1 = (char *)c; // Old-style cast OK, Obscure not traceable
char * p2 = static_cast<char >(c); // Error: static_cast from type 'const char * ' to type 'char * '
char * p3 = reinterpret_cast<char * >(c); // Error: reinterpret_cast from type 'const char' to type 'char * ' casts away qualifiers
char * p4 = const_cast<char * >(c); // OK -> But traceable not obsure if you know wat you are doing

My point is to avoid using magic words when they are obscur.

They are not more magic then if/switch/struct etc if you know the C++ language

Edit: sorry the code was corrupted
But I hope you get the idea of the above code and rethink you opinion about the magic

[Barracuda09]

So, short story with 'old-style' cast you let the compiler decide wat he thinks your intension is

[d-a-v]

What I'm trying to say is that

• I feel *_cast<> are useless except when we are dealing with inheritance (like A* a = new SonOfA; SonOfA* soa = dynamic_cast<SonOfA*>(a);).
• Code is much more readable without them.
• I don't consider char x = 1; int y = int(x); or char x = 'a'; String y = String(x); old-style cast.
• I don't consider writing those is worse than using *_cast<> in terms of performances.

I'm still looking for good reasons to not think like this.

[Barracuda09]

@d-a-v

I feel _cast<> are useless except when we are dealing with inheritance (like A a = new SonOfA; SonOfA* soa = dynamic_cast<SonOfA*>(a);).

'old style' cast can do this also if you let the compiler decide... But it is not traceable

Code is much more readable without them.

That should not be, because it is something unusual (don't say it is more typeing then you use also an 'old-style' editor)

don't consider char x = 1; int y = int(x); or char x = 'a'; String y = String(x); old-style cast.

Well these are not cast they are constructors, BUT do not use int(x) that does not make sense

I don't consider writing those is worse than using *_cast<> in terms of performances.

performence is indeed debatable.

But the hole idea is that you can traceback these anomalies in the code. With 'old-style' it is not.
But if you still find it not nessasery to get rid of the 'old-style' cast, I will leave you with your thoughts

[d-a-v]

with 'old-style' cast you let the compiler decide wat he thinks your intension is

It seems more right to me to say that *_cast<> makes the compiler decide what to do, that's why I called them "magical" keywords.
I am doing everything to stay away from this. I want and need to know what exactly the compiler does, and the initial patch (old style, yes but could be c++ style like point 3 above) is intended to tell and guide the compiler to do what I want.

[Barracuda09]

@d-a-v

Sorry, You lost me here

[devyte]

Alright, in the words of my countrymen: "Somebody has to cut the cake".
@Barracuda09 is correct in that set of examples. Using C-style cast essentially lets the compiler decide what kind of cast to do. This could potentially allow a cast that you did not really intend, in which case if you make a mistake, the code could build into a bug. Prime example:

const char c = "Hello world";
char p1 = (char*)c;

There are only a very few cases where one would want to cast away a const qualifier, and in those cases it is better to be made explicit to show that it is on purpose. Hence, use of const_cast<>() is better in such a cases.

dynamic_cast<>() should be used only with polymorphic classes.

reinterpret_cast<>() exists for memory transformations. The use case is similar to a union, where you write to one type and read from the other type. Example:

unsigned int num = 0xFFFFFFFF;
unsigned char *ptr = reinterpret_cast<unsigned char*>(&num);
ptr[0] = 0;

Again, this is meant to make the intent explicit: I am taking an address to an unsigned int, and interpreting the data it points to as something different.

static_cast<>() is meant to replace the typical intent of old C-style cast, while disallowing unintended cases like casting away the const qualifier. For casting between different sizes of integer, this would be appropriate.

So, yes: the *_cast<>() versions

• are verbose, and they do reduce readability a bit at a glance
• are explicit, and reduce chances of introducing unintended bugs (this is the main reason they exist)
• are considered better programming style in C++
• can be overkill in some cases, but it's like some other C++ things I try to drill into peers: it's best to get into the habit of using them, i.e.: if you don't use them, you better know exactly what you're doing.

[devyte]

Now, let's get the thread back to the original issue reported here, which is the overload ambiguity.

[Barracuda09]

@devyte

Thanks for you explanition, hope @d-a-v will change his/her mind.

[devyte]

To be clear, my previous explanation is the general explanation and recommendation.
It takes a while to understand the underlying reasons, and it's hard to change the habit. Even with what I know, I still have the urge to use C-style casts sometimes for some of the same reasons that @d-a-v says. I may even have a few in my ESP app, but I'll never admit to it 😜
And like I said, you can still use C-style casts if you know what you're doing. I think that an int-to-char cast where there is no templated type is trivial enough /shrug

[d-a-v]

hope @d-a-v will change his/her mind.

My mind tells me size_t write(int i) { return write(uint8_t(i)); } is not old-style C-cast.

From Bjarne Stroustrup:

An ugly operation should have an ugly syntactic form

Thank you to help me discovering this today :)

@Barracuda09 you are free to make a PR, I am no authority here !

[Barracuda09]

@d-a-v

Well I am still learning as well, and I am far from a teacher. So I want to apologize for my approach.

[d-a-v]

So I want to apologize for my approach.

Don't. I am a part-time c++ teacher, so if there are good arguments to convince me, do it quick, because I would otherwise spread evil knowledge ;-)

[devyte]

@TD-er ESPEasy is calling f.write(0). That "0" is an int, and there is no write() method that takes an int as argument, i.e.: that is not part of the class interface. Therefore, the compiler is searching for the next best thing, which is an implicit cast of the "0" one step away from the type.
In prior code, it happened to find File::write(uint8_t), and it's the only* overload, and so it's not ambiguous, but that's pure luck.
In current code, a new overload File::write(const char *) is added*, and that is also one step away, so now there are two overloads found => ambiguous.

The correct thing to do is to explicitly call with a supported overload, like one of the following:

f.write(char(0))
f.write((char)0)

*Notice that there seems to be a bug in the core, because the new File::write(const char *str) is a reimplementation of the base class Print::write(const char *str), which has existed for a long time, with almost the exact same code. However, that base class method is missing the virtual keyword. I suspect that this bug is the only reason that calling f.write(0) worked before. What I think should have happened is that the virtual keyword should have existed, and then when ESPEasy tried to originally call f.write(0), the ambiguous compile error would have popped up immediately, i.e.: the current usage by ESPEasy with an int arg should never have worked.

[d-a-v]

While I agree, I'm afraid ESPEasy's issue is only the first one with this API change.
We need to maintain backward compatibility or bad stuff will come back later, with teeth #5749 (especially the third point)

[TD-er]

I totally agree on the ugliness of that part of the code and I am also willing to change it, but like @d-a-v suggests, it may lead to a lot of other projects failing.

As a quick work-around I already copied the byte to be written to a new uint8_t value variable and then call f.write(&value, 1) to work around this issue with a // FIXME TD-er statement along with it to make a proper fix for that part of the code.

N.B. Before trying to fix it, I also tried to write(0u), but that also didn't work. Was still ambigu according to GCC.

[JAndrassy]

Arduino base class Print has 4 write functions:

    virtual size_t write(uint8_t) = 0;
    size_t write(const char *str) {
      if (str == NULL) return 0;
      return write((const uint8_t *)str, strlen(str));
    }
    virtual size_t write(const uint8_t *buffer, size_t size);
    size_t write(const char *buffer, size_t size) {
      return write((const uint8_t *)buffer, size);
    }

File implements Print (Stream)
every good Arduino class that implements Print has:

    using Print::write; // pull in write(str) and write(buf, size) from Print

and it works

[TD-er]

@JAndrassy Have you already tested it like that?

[JAndrassy]

I find so many bugs and misunderstandings in Arduino cores and libraries that I can't take care of all of them

this is how SD File, HardwareSerial, SoftwareSerial, net Client implementations, LCD libraries have it

[Barracuda09]

@d-a-v

Trying one more time to convince you (Last attept).
This is a more complex situation, which will look very nice with 'old-style' casts.

Look at this code:

const char tmp[] = "Hello World";

const char    *p1 = tmp;            // Ok, same type and constness (No cast needed)
char          *p2 = (char *)tmp;    // Ok "Looks nice not traceable", but cast away const
uint8_t       *p3 = (uint8_t *)tmp; // OK "Looks nice not traceable", but does reinterpret cast and cast away constness
const uint8_t *p4 = tmp;            // Error: Not same type but same constness 
uint8_t       *p5 = reinterpret_cast<uint8_t *>(tmp); // Error: can not cast away constness
uint8_t       *p6 = const_cast<uint8_t *>(tmp);       // Error: can not reinterpret cast
uint8_t       *p7 = reinterpret_cast<uint8_t *>(const_cast<char *>(tmp)); // Ok Same as p3, but looks bud ugly but if this is what you want, lets do it then

So please rethink and try to use *_cast<>

[earlephilhower]

@TD-er, all: Can you please give #5861 a try? It should preserve the old behavior (i.e. no need to add casts to your older code).

[TD-er]

The last change made the error move to another class, the write part of HardwareSerial.

/home/travis/build/letscontrolit/ESPEasy/src/_P054_DMX512.ino: In function 'boolean Plugin_054(byte, EventStruct*, String&)':
/home/travis/build/letscontrolit/ESPEasy/src/_P054_DMX512.ino:250:26: error: call of overloaded 'write(int)' is ambiguous
Serial1.write(0);   //start byte
^
/home/travis/build/letscontrolit/ESPEasy/src/_P054_DMX512.ino:250:26: note: candidates are:
In file included from /home/travis/.platformio/packages/framework-arduinoespressif8266@src-31d658a59f41540201fc3726a1394910/cores/esp8266/Arduino.h:263:0,
from /tmp/tmp_7sgaG:1:
/home/travis/.platformio/packages/framework-arduinoespressif8266@src-31d658a59f41540201fc3726a1394910/cores/esp8266/HardwareSerial.h:151:12: note: virtual size_t HardwareSerial::write(uint8_t)
size_t write(uint8_t c) override
^
/home/travis/.platformio/packages/framework-arduinoespressif8266@src-31d658a59f41540201fc3726a1394910/cores/esp8266/HardwareSerial.h:159:12: note: size_t HardwareSerial::write(const char*)
size_t write(const char *buffer)
^
*** [.pioenvs/test_core_260_sdk2_alpha_ESP8266_4M/src/ESPEasy.ino.cpp.o] Error 1

For ESPeasy I will fix it, but it may be others will also report some similar issues.

[earlephilhower]

@devyte , did we get the hierarchy wrong? I don't think so, Print -> Stream -> (HW Serial, FS). So wouldn't the write methods at the lower level disambiguate at the uppermost one?

[TD-er]

Please note that the Travis output this error appeared in was still having the changed code I added to work around the previous issue.
So it is possible the call with 0 as argument to write() is still causing issues in File::write but we don't see it now.
And because the code has now been moved, that other derived classes may also suffer from the same issue.

[earlephilhower]

I have added host tests which include File.write(0) (and others), so I feel pretty confident that bit works. But there's obviously no HWUart host tests, so that wasn't tested. The class was just missing the "using" statement. Manual checks show Serial.write(0/other ints) and (C-string) look good now.

[TD-er]

Great, then I will restart that Travis build, so we'll see :)

[dvukovic]

This issue has been closed, but it has not been fixed.
Should I go back to a former version ?

[d-a-v]

#5878 is supposed to have fixed it. Please try latest master, and open a new issue with your details.

[dvukovic]

[dvukovic]

Multiple libraries were found for "SdFat.h"
Used: C:\Users\Donald\Documents\Arduino\libraries\SdFat
Not used: C:\Users\Donald\AppData\Local\Arduino15\packages\esp8266\hardware\esp8266\2.5.2\libraries\ESP8266SdFat
Using library SdFat at version 1.1.0 in folder: C:\Users\Donald\Documents\Arduino\libraries\SdFat
exit status 1
call to 'fs::File SDClass::open(const char*, uint8_t)' uses the default argument for parameter 2, which is not yet defined
So I deleted C:\Users\Donald\Documents\Arduino\libraries\SdFat
And deleted C:\Users\Donald\AppData\Local\Arduino15\packages\esp8266\hardware\esp8266\2.5.2\libraries\ESP8266SdFat