Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix lost error message #1370

Merged
merged 4 commits into from
Nov 13, 2024
Merged

fix lost error message #1370

merged 4 commits into from
Nov 13, 2024

Conversation

tsteven4
Copy link
Collaborator

@tsteven4 tsteven4 commented Nov 6, 2024

Why this compiled and output nothing is a mystery to me.

You can test with:
gpsbabel -i kml -f reference/bounds-test.kml -o gpx,gpxver=0.9 -F /dev/null
which should produce a message
GPX: gpx version number "0.9" not valid.

@tsteven4
Copy link
Collaborator Author

tsteven4 commented Nov 6, 2024

This is another way to "fix" it, and a clue to what is wrong:

-    fatal(FatalMsg() << MYNAME ": gpx version number"
-          << gpx_write_version.toString() << "not valid.");
+    qCritical() << MYNAME ": gpx version number"
+          << gpx_write_version << "not valid.";

This results in a message
GPX: gpx version number QVersionNumber(0.9) not valid.

@robertlipe
Copy link
Collaborator

LGTM

The best thing about streaming operators is implicit conversions.
The worst thing about streaming operators is implicit conversions.

Related thought. Now that we're c++20, we can rely on std::fmt, no? I'm not sure I want to go nuts replacing printf specifiers everywhere, (in fact, I'm pretty sure I don't...) I resisted it when it meant bringing in a thirdparty (even though that version IS the implementation in most platforms) library, but for debugging, in particular, it can be pretty nice. I'm not sure how well it plays with Qt types[1], but it can't be worse than printf...plus fmt has been pretty widely embraced through the industry so it seems likely that someone has

Compile-time format argument matching testing is pretty nice if we can rely on it.

I'd even give some consideration to exposing these formatting specifiers in our style definitions, but I'm nto really sure how many people are writing their own.

[1] See https://bugreports.qt.io/browse/QTBUG-104651, though that seems mostly internal to Qt itself. Also https://wiki.qt.io/QtCS2024_std::format - notably QTBUG-104652 which dashes my hope above, but looks trivial to both patch in and to disable when our floor hits Qt 6.8 or whatever.

@tsteven4
Copy link
Collaborator Author

tsteven4 commented Nov 7, 2024

This is related to our use of the undocumented operator overload from qversionnumber.h
Q_CORE_EXPORT QDebug operator<<(QDebug, const QVersionNumber &version);
Note the operator operates on and returns a QDebug object, not a QDebug object reference.

If the QVersionNumber was the last thing sent then the code doesn't even compile

/.../gpx.cc: In member function ‘virtual void GpxFormat::wr_init(const QString&)’:
/.../gpx.cc:1003:11: error: cannot bind non-const lvalue reference of type ‘QDebug&’ to an rvalue of type ‘QDebug’
 1002 |     gbFatal(FatalMsg() << "gpx version number"
      |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1003 |           << gpx_write_version);
      |           ^~~~~~~~~~~~~~~~~~~~
In file included from /.../gpx.h:34,
                 from /.../gpx.cc:22:
/.../defs.h:913:35: note:   initializing argument 1 of ‘void gbFatal(QDebug&)’
  913 | [[noreturn]] void gbFatal(QDebug& msginstance);
      |                           ~~~~~~~~^~~~~~~~~~~
[2/3] Building CXX object CMakeFiles/gpsbabel.dir/tpo.cc.o

@tsteven4
Copy link
Collaborator Author

tsteven4 commented Nov 7, 2024

I'm not sure I want to go nuts replacing printf specifiers everywhere, (in fact, I'm pretty sure I don't...)

Me either!

@tsteven4 tsteven4 merged commit 40aad44 into GPSBabel:master Nov 13, 2024
18 checks passed
@tsteven4 tsteven4 deleted the gpxmsg branch November 13, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants