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