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

Source location in Debug #55

Merged
merged 2 commits into from Nov 26, 2019
Merged

Source location in Debug #55

merged 2 commits into from Nov 26, 2019

Conversation

@mosra
Copy link
Owner

mosra commented Jan 21, 2019

Similarly to the new dbg! macro in Rust, this implements source location for Debug using std::experiemntal::source_location. At the moment this feature is present only in GCC 8.1 / libstdc++ and up and a builtin in Clang 9:

This is an experiment that needs further design iterations:

  • At the moment, all Debug{}, Error{} etc. constructors are acquiring the source location and saving it even if it might not be used, which means extreme increase in binary sizes. While this is convenient and doesn't require changes on the user side (it's just enabling a flag and then all following debug statements in the same scope get source location), it's not efficient. Some ideas:
    • Provide a dedicated function such as debug() that instantiates Debug and with source location (bad, it requires users to learn a new thing, also we would need to do an error(), fatal() etc. and there's a high chance these names will clash with user code)
    • Provide a constructor tag such as Debug{Debug::SourceLocation} (again kinda bad, since it's verbose to say every time)
    • Save source file name only at the point of enabling the debug output but save line in every constructor (would need some way to detect what file we are in in order to not print file info from unrelated files, maybe abusing unnamed namespaces?)
    • Provide a constructor tag that explicitly does not use source location and then use that in all the library internals to minimize the impact (goes against "don't pay for what you don't use", doesn't really help users)
    • Use !Debug{}
  • The Rust dbg! macro works inside if() as well, provide something like that too? not sure how
  • The GCC implementation requires C++14, I'm working around that by #undef constexpr for C++11 (which is insane), submit a PR to libstdc++ that uses some C++14 constexpr instead? -- using the builtins directly
  • The file+line info output format is probably not the best (use colors? wrap in [] like Rust does? make it format()able, allowing stuff like column, date and other things there?) postponed, will be done together with arbitrary line prefixing
  • Some way to print just file & line without anything else like Rust 1.35 has

Obligatory docs:

image

Further design ideas welcome! :)

@mosra mosra added this to TODO in Utility via automation Jan 21, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 21, 2019

Codecov Report

Merging #55 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #55      +/-   ##
=========================================
+ Coverage   97.49%   97.5%   +<.01%     
=========================================
  Files          87      81       -6     
  Lines        5796    5442     -354     
=========================================
- Hits         5651    5306     -345     
+ Misses        145     136       -9
Impacted Files Coverage Δ
src/Corrade/Utility/Debug.cpp 96.75% <ø> (+0.96%) ⬆️
src/Corrade/Utility/Debug.h 100% <100%> (ø) ⬆️
src/Corrade/Interconnect/Connection.cpp 36.36% <0%> (-63.64%) ⬇️
...rrade/TestSuite/Implementation/BenchmarkCounters.h 88.88% <0%> (-11.12%) ⬇️
src/Corrade/Utility/FileWatcher.cpp 96.66% <0%> (-3.34%) ⬇️
src/Corrade/Utility/Arguments.cpp 98.06% <0%> (-1.94%) ⬇️
src/Corrade/Utility/Unicode.cpp 98.09% <0%> (-1.91%) ⬇️
src/Corrade/Utility/Directory.cpp 88.46% <0%> (-0.47%) ⬇️
src/Corrade/TestSuite/Tester.cpp 93.36% <0%> (-0.31%) ⬇️
src/Corrade/PluginManager/AbstractManager.cpp 97.66% <0%> (-0.25%) ⬇️
... and 50 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5c02d9a...3872b3c. Read the comment docs.

@mosra mosra mentioned this pull request Feb 24, 2019
3 of 4 tasks complete
@mosra mosra mentioned this pull request Mar 10, 2019
4 of 4 tasks complete
@mosra

This comment has been minimized.

Copy link
Owner Author

mosra commented May 23, 2019

Rust 1.35 has an empty dbg! macro to print just the file & line. Added that to the list above.

@mosra mosra force-pushed the source-location branch from 95d056e to a59db0a Jun 27, 2019
@mosra mosra changed the title [WIP] Source location in Debug Source location in Debug Jun 27, 2019
@mosra

This comment has been minimized.

Copy link
Owner Author

mosra commented Jun 27, 2019

Invented a much better version that's opt-in, easy to use and doesn't bloat the binary when not used. The last remaining blocker is #define constexpr, but I might just ignore that problem for now.

@mosra mosra added this to the 2019.0b milestone Jun 27, 2019
@mosra mosra force-pushed the source-location branch from a59db0a to e4c1361 Jun 27, 2019
@mosra mosra modified the milestones: 2019.0b, 2019.0c Oct 2, 2019
@mosra

This comment has been minimized.

Copy link
Owner Author

mosra commented Oct 2, 2019

To have this working on both Clang and GCC, I'll probably change to using the compiler builtins, and that'll solve the #define constexpr as well.

Supported on GCC 8.1+ and Clang 9+.
@mosra mosra force-pushed the source-location branch from e4c1361 to 3872b3c Nov 26, 2019
You're the constant source of PAIN in my life.
@mosra mosra merged commit c1e884b into master Nov 26, 2019
2 of 4 checks passed
2 of 4 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Utility automation moved this from TODO to Done Nov 26, 2019
@mosra mosra deleted the source-location branch Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Utility
  
Done
2 participants
You can’t perform that action at this time.