PrintFile and StreamFile warnings with -Woverloaded-virtual

[mcspr]

Building with esp8266/Arduino, gcc10.3 and forcibly enabled -Woverloded-virtual

In file included from cores/esp8266/Stream.h:27,
                 from cores/esp8266/HardwareSerial.h:32,
                 from cores/esp8266/Arduino.h:303,
                 from libraries/SD/src/SD.h:23,
                 from libraries/SD/src/SD.cpp:1:
libraries/ESP8266SdFat/src/ExFatLib/../common/ArduinoFiles.h: In instantiation of 'class PrintFile<FatFile>':
libraries/ESP8266SdFat/src/SdFat.h:455:23:   required from here
cores/esp8266/Print.h:61:24: warning: 'virtual size_t Print::write(const uint8_t*, size_t)' was hidden [-Woverloaded-virtual]
   61 |         virtual size_t write(const uint8_t *buffer, size_t size);
      |                        ^~~~~
In file included from libraries/ESP8266SdFat/src/FatLib/FatVolume.h:27,
                 from libraries/ESP8266SdFat/src/FatLib/FatLib.h:27,
                 from libraries/ESP8266SdFat/src/SdFat.h:34,
                 from libraries/SDFS/src/SDFS.h:35,
                 from libraries/SD/src/SD.h:25,
                 from libraries/SD/src/SD.cpp:1:
libraries/ESP8266SdFat/src/FatLib/FatFile.h:913:10: note:   by 'PrintFile<FatFile>::write'
  913 |   size_t write(const char* str) {
      |          ^~~~~

...etc...

If I understood the intent correctly, changes should be something like this - hiding original methods by removing 'using = ...' and adding explicit override to the methods required for Print & Stream
mcspr/ESP8266SdFat@f4d4076
(we are still at SdFat version 2.1.1, but the issue applies to both latest release and beta version here. plus, updated formatting)

Do you accept pull-requests here?

[greiman]

Do you accept pull-requests here?

I would rather have the the patch so I can apply it to my development version.

I am about to post a new beta which has much new code. I like to test mods on many boards before posting and I am about to test the new beta.

If will I get these mods if I just use this version of ArduinoFiles.h?

[greiman]

I took your version of ArduinoFiles.h and ran it though clang_format since I now don't fuss with formatting during development and use clang_format.

I modified the current version of SdFat-beta ArduinoFiles.h so:

diff -w ArduinoFiles_mcpr_clang.h ArduinoFiles.h 1>diff.txt

Is

2c2
<  * Copyright (c) 2011-2021 Bill Greiman
---
>  * Copyright (c) 2011-2024 Bill Greiman
27c27
< #include "SdFatConfig.h"
---
> #include "SysCall.h"
85a86,87
> 
> #ifndef DOXYGEN_SHOULD_SKIP_THIS
89d90
<    * \return a pointer to replacement suggestion.
91d91
< #ifndef DOXYGEN_SHOULD_SKIP_THIS
94d93
<   const char* name() const { return "use getName()"; }
108d106
< 
117d114
< 

[mcspr]

Re-uploaded after running clang-format
mcspr/ESP8266SdFat@85e0666
https://github.com/mcspr/ESP8266SdFat/commit/85e06665a1f26e548a42baaa412b0d95281db270.diff
https://github.com/mcspr/ESP8266SdFat/commit/85e06665a1f26e548a42baaa412b0d95281db270.patch

I also forgot about Print::flush() in the first one

[greiman]

I also forgot about Print::flush() in the first one

I added Print::flush() and now your fix-wov/beta matches the version I am testing for the next SdFat-beta.

The next SdFat-beta will have have an RP2040 PIO SDIO driver I wrote and can read/write at about 25 MB/sec for small, transfers.

I tried the ESP32 SDIO driver but it is not worth adding since there is no way to make it go fast for transfers smaller than about 16 KB. Then it is still fairly slow.

Unfortunately most SDIO drivers were written for FatFs and don't have a way to use the code I wrote for shared SPI. I probably will only write new SDIO drivers for MCUs I use.

[greiman]

I started testing and found a problem. I searched through 21 Arduino packages for the regular expression "virtual +int +read" in Stream.h and found ESP8266 was the only one that had virtual int read(uint8_t*, size_t).

  C:\Users\Bill\AppData\Local\Arduino15\packages\adafruit\hardware\samd\1.7.14\cores\arduino\Stream.h (1 hit)
	Line  60:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\arduino\hardware\avr\1.8.6\cores\arduino\Stream.h (1 hit)
	Line  60:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\arduino\hardware\mbed_giga\4.1.1\cores\arduino\api\Stream.h (1 hit)
	Line  61:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\arduino\hardware\mbed_rp2040\4.1.1\cores\arduino\api\Stream.h (1 hit)
	Line  61:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\arduino\hardware\megaavr\1.8.8\cores\arduino\api\Stream.h (1 hit)
	Line  61:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\arduino\hardware\renesas_portenta\1.1.0\cores\arduino\api\Stream.h (1 hit)
	Line  61:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\arduino\hardware\renesas_uno\1.1.0\cores\arduino\api\Stream.h (1 hit)
	Line  61:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\arduino\hardware\sam\1.6.12\cores\arduino\Stream.h (1 hit)
	Line  60:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\arduino\hardware\samd\1.8.14\cores\arduino\api\Stream.h (1 hit)
	Line  61:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\ATTinyCore\hardware\avr\1.5.2\cores\tiny\Stream.h (1 hit)
	Line  60:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\ATTinyCore\hardware\avr\1.5.2\cores\tinymodern\Stream.h (1 hit)
	Line  60:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\esp32\hardware\esp32\2.0.11\cores\esp32\Stream.h (1 hit)
	Line  49:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\esp8266\hardware\esp8266\3.1.2\cores\esp8266\Stream.h (3 hits)
	Line  41: // Arduino `Client: public Stream` class defines `virtual int read(uint8_t *buf, size_t size) = 0;`
	Line  60:         virtual int read() = 0;
	Line 119:         virtual int read (uint8_t* buffer, size_t len);
  C:\Users\Bill\AppData\Local\Arduino15\packages\MightyCore\hardware\avr\3.0.1\cores\MCUdude_corefiles\Stream.h (1 hit)
	Line  60:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\MiniCore\hardware\avr\3.0.1\cores\MCUdude_corefiles\Stream.h (1 hit)
	Line  60:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\rp2040\hardware\rp2040\3.7.2\ArduinoCore-API\api\Stream.h (1 hit)
	Line  61:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\STMicroelectronics\hardware\stm32\2.7.1\cores\arduino\Stream.h (1 hit)
	Line  59:     virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\teensy\hardware\avr\1.59.0\cores\teensy\Stream.h (1 hit)
	Line 31: 	virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\teensy\hardware\avr\1.59.0\cores\teensy3\Stream.h (1 hit)
	Line 33: 	virtual int read() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\teensy\hardware\avr\1.59.0\cores\teensy4\Stream.h (1 hit)
	Line 33: 	virtual int read() = 0;

These seem to be the standard virtual member function in true Arduino packages:

  C:\Users\Bill\AppData\Local\Arduino15\packages\arduino\hardware\renesas_uno\1.1.0\cores\arduino\api\Print.h (4 hits)
	Line 49:     virtual size_t write(uint8_t) = 0;
	Line 54:     virtual size_t write(const uint8_t *buffer, size_t size);
	Line 61:     virtual int availableForWrite() { return 0; }
	Line 92:     virtual void flush() { /* Empty implementation for backward compatibility */ }
  C:\Users\Bill\AppData\Local\Arduino15\packages\arduino\hardware\renesas_uno\1.1.0\cores\arduino\api\Stream.h (3 hits)
	Line  60:     virtual int available() = 0;
	Line  61:     virtual int read() = 0;
	Line  62:     virtual int peek() = 0;

I decided to try changing:

  int read(uint8_t* buffer, size_t size) override {
    return BaseFile::read(buffer, size);
  }

to:

  int read(void* buf, size_t size) {
    return BaseFile::read(buf, size);
  }

Then it matches other files:

  C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\src\common\ArduinoFiles.h (1 hit)
	Line 115:   int read(void* buf, size_t size) {
  C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\src\ExFatLib\ExFatFile.h (1 hit)
	Line 648:   int read(void* buf, size_t count);
  C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\src\FatLib\FatFile.h (1 hit)
	Line  796:   int read(void* buf, size_t count);
  C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\src\FsLib\FsFile.h (1 hit)
	Line 699:   int read(void* buf, size_t count) {

[mcspr]

Hm... this was added for StreamDev.h API in ESP8266 (previosuly virtual size_t readBytes()), so it is definitely not real-real Arduino API. We can change it to have if defined(ESP8266) in our fork, though. So not a big problem, when read(void*) is available

diff --git a/src/common/ArduinoFiles.h b/src/common/ArduinoFiles.h
index 4c39dad..0aabf09 100644
--- a/src/common/ArduinoFiles.h
+++ b/src/common/ArduinoFiles.h
@@ -106,9 +106,20 @@ class StreamFile : public stream_t, public BaseFile {
    * \return For success return the number of read bytes.
    * If an error occurs or end of file is reached return -1.
    */
-  int read(uint8_t* buffer, size_t size) override {
+  int read(void* buffer, size_t size) {
     return BaseFile::read(buffer, size);
   }
+#if defined(ESP8266)
+  int read(uint8_t* buffer, size_t size) override {
+    return BaseFile::read((void*)buffer, size);
+  }
+  size_t readBytes(uint8_t* buffer, size_t size) override {
+    return (size_t)read((void*)buffer, size);
+  }
+  size_t readBytes(char* buffer, size_t size) {
+    return readBytes((uint8_t*)buffer, size);
+  }
+#endif
   /** Rewind a file if it is a directory */
   void rewindDirectory() {
     if (BaseFile::isDir()) {

[greiman]

Another problem. virtual void flush() is missing in some packages.

c:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\src/ExFatLib/../common/ArduinoFiles.h:49:8: error: 'void PrintFile::flush() [with BaseFile = FsBaseFile]' marked override, but does not override
void flush() override { BaseFile::sync(); }
^

The big problem is Arduino Due. These are the only virtual members.

 C:\Users\Bill\AppData\Local\Arduino15\packages\arduino\hardware\sam\1.6.12\cores\arduino\Print.h (2 hits)
	Line 48:     virtual size_t write(uint8_t) = 0;
	Line 53:     virtual size_t write(const uint8_t *buffer, size_t size);

The new standard Arduino API has these four virtual members.

  C:\Users\Bill\AppData\Local\Arduino15\packages\arduino\hardware\renesas_uno\1.1.0\cores\arduino\api\Print.h (4 hits)
	Line 49:     virtual size_t write(uint8_t) = 0;
	Line 54:     virtual size_t write(const uint8_t *buffer, size_t size);
	Line 61:     virtual int availableForWrite() { return 0; }
	Line 92:     virtual void flush() { /* Empty implementation for backward compatibility */ }

I am thinking of removing it from class PrintFile since I put it in many years ago before Arduino removed it from Print and put it in Stream.

It is in the base files so it should not break apps

 C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\src\ExFatLib\ExFatFile.h (1 hit)
	Line 260:   void flush() { sync(); }
  C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\src\FatLib\FatFile.h (1 hit)
	Line  341:   void flush() { sync(); }
  C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\src\FsLib\FsFile.h (1 hit)
	Line 256:   void flush() { sync(); }

Any suggestions?

P.S. You can see why I don't blindly use pull-requests. I find almost all pull-requests tested on only one board package break some other board package.

[greiman]

Here is the history of flush from Arduino.

Strange they say this:
flush() inherits from the Stream utility class.

Since it is rarely in the Stream class.

  C:\Users\Bill\AppData\Local\Arduino15\packages\arduino\hardware\sam\1.6.12\cores\arduino\Stream.h (1 hit)
	Line  62:     virtual void flush() = 0;
  C:\Users\Bill\AppData\Local\Arduino15\packages\teensy\hardware\avr\1.59.0\cores\teensy\Stream.h (1 hit)
	Line 33: 	virtual void flush() = 0;

This explains why Due has the error for flush() in Print. Flush should be in Print but the main use of flush is in Serial which has Stream as the base class and Stream has Print as the Base class. So for implementations of Serial flush can be in either Print or Stream.

Maybe I can remove flush from ArduinoFiles.h and not break anything.

[mcspr]

flush() can lose override keyword, apparently C++ does not care whether it is there (I did care, though :)
https://en.cppreference.com/w/cpp/language/virtual#In_detail

But warnings would still trigger for different arguments in similarly named methods

[greiman]

flush() can lose override keyword, apparently C++ does not care whether it is there (I did care, though :)

I understand.

Maybe the answer is to have the override in the StreamFile class and not the PrintFile class. A C++ compiler can't know whether flush appears in Stream or Print since Stream has Print as a base class.

Try this and see if you get warnings.

I only have the PrintFile class for backward compatibility with the old SdFile class. I don't use it in any version 2.x examples.

  C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\src\common\ArduinoFiles.h (3 hits)
	Line  39:  * \class PrintFile
	Line  40:  * \brief PrintFile class.
	Line  43: class PrintFile : public print_t, public BaseFile {
  C:\Users\Bill\Documents\ArduinoSdFat\libraries\SdFat\src\SdFat.h (1 hit)
	Line 468: class SdFile : public PrintFile<SdBaseFile> {

There are now over 100 board packages for the Arduino IDE.

It is hopeless to write a modern safe C++ library for this motley collection.

[mcspr]

No warnings w/ the suggested change for PrintFile. I do see now that StreamFile is what ESP8266 have been using through File32, PrintFile just got noticed at the same time since it gets pulled from SdFat.h as well

btw I only noticed this warning after trying to build our Core tests with gcc13.2, which happen to include SdFat to check SDFS & SD APIs. By default it builds with -Wall -Werror, and -Woverloaded-virtual=1 is now enabled by default with >=gcc13 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87729

(-Woverloaded-virtual=2 also passes the build, only errors come from ESP8266 classes)

[greiman]

I decided to make a change so PrintFile builds correctly. I added an exception for SAM_DUE. The board package for Due clearly has a bug with flush() being in Stream and not in Print.

I may post an issue about flush() for Due. Arduino probably won't bother to fix it since no one has posted an issue about flush() not being in Print.

This is my mod for flush() in PrintFile:

  /** Ensure that any bytes written to the file are saved to the SD card. */
#if defined(ARDUINO_SAM_DUE) && !defined(ARDUINO_API_VERSION)
  void flush() { BaseFile::sync(); }
#else
  void flush() override { BaseFile::sync(); }
#endif

This is flush() in StreamFile:

  /** Ensure that any bytes written to the file are saved to the SD card. */
  void flush() override { BaseFile::sync(); }

Arduino is modifying most board packages to use this new ArduinoCore-API package. It has flush() here.

If the Due package is updated, the above patch should still work.

Here is a zip with the current version of ArduinoFiles.h
ArduinoFiles.zip