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

Refactor error_helper and around #4686

Merged
merged 12 commits into from
Jun 2, 2024
Merged

Refactor error_helper and around #4686

merged 12 commits into from
Jun 2, 2024

Conversation

asl
Copy link
Contributor

@asl asl commented May 27, 2024

This is a next step around messages emission refactoring. Essentially:

  • Use C++17 features to reduce code duplication and modernize code, use universal references instead of passing by value
  • Get rid of varargs for parser errors, use variadic templates instead
  • Simplify code here and there getting rid of cstrings
  • Unify object => string conversion

Things are a bit messy for the latter. We are having: toString, dbprint (that are different) and sometimes explicit operator<<. Plus, we define operator<< in a global namespace (in log.g, and some IR classes depend on this!) and call dbprint from there. However, for error messages, etc. we do not want to use operator<< (that is default implementation for boost::format for user-defined types) and use explicit toString.

So, this PR tries to do some consolidation of conversion to strings:

  • Everything is moved to stringify.h, so log.h is all about logging
  • We define operator<< to use toString, if available. If not, we try to delegate to dbprint

@asl asl requested review from vlstill and ChrisDodd May 27, 2024 23:27
@ChrisDodd
Copy link
Contributor

ChrisDodd commented May 27, 2024

  • We define operator<< to use toString, if available. If not, we try to delegate to dbprint

This seems backwards as operator<< is mostly (only?) used for logging, so it should try to use dbprint in preference to toString -- toString is for user-facing messages (removes any interal cruft) while dbprint is for logging/debugging (includes the internal cruft as that may be important for understanding what is going on). Changing this will significantly impact logging, making it less useful.

@asl
Copy link
Contributor Author

asl commented May 27, 2024

This seems backwards as operator<< is mostly (only?) used for logging,

That's exactly the problem :(

  1. boost::format uses operator<< for user types. So, we'd either need to be explicit with toString, or we'll end with dbprint output
  2. Some code really does operator<< for user-facing things

So, maybe, we'd be explicit? And remove operator<< at all? So, there will be dbprint and toString and they will not mix with each other?

@ChrisDodd
Copy link
Contributor

ChrisDodd commented May 28, 2024

This seems backwards as operator<< is mostly (only?) used for logging,

That's exactly the problem :(

  1. boost::format uses operator<< for user types. So, we'd either need to be explicit with toString, or we'll end with dbprint output
  2. Some code really does operator<< for user-facing things

So, maybe, we'd be explicit? And remove operator<< at all? So, there will be dbprint and toString and they will not mix with each other?

Ugh -- that would require rewriting all of the LOG statements everywhere, including all the backends (which tend to rely on them quite a bit). That was why we had error_helper explicitly try to call toString where possible rather than using boost::format.

Maybe we could add a DBPrint::error_msg flag and have dbprint fall back to toString style cleanup for user-facing error messages and use that in error_helper to avoid getting cruft in error messages (the BUG messages should probably continue to use dbprint as those aren't really user-facing, so detail is better there -- "clean" release builds tend to catch CompilerBug exceptions and print a generic "Internal Error" message rather than the BUG message.)

@asl
Copy link
Contributor Author

asl commented May 28, 2024

@ChrisDodd Ok. I moved to opposite direction (dbprint then toString). Let's see where we will land in the end!

@asl asl requested a review from ChrisDodd May 28, 2024 00:29
@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label May 28, 2024

#include "big_int_util.h"
// FIXME: Replace with big_int_fwd.h with Boost 1.84+
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#4663 could help with this.

@fruffy fruffy added run-ubuntu18 Use this tag to trigger a Ubuntu-18 CI run. run-sanitizer Use this tag to run a Clang+Sanitzers CI run. run-static Use this tag to trigger static build CI run. labels May 30, 2024
@asl
Copy link
Contributor Author

asl commented May 31, 2024

@fruffy @ChrisDodd @vlstill

Will you please take a look when you will have a chance? I am having other PRs in the pipeline that are related to various strings aspects and they are a bit conflicting with each other to keep everything downstream :)

Thanks!

Copy link
Contributor

@ChrisDodd ChrisDodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be ok, but I think the idea of unifying toString and dbprint with an additional flag in dbprint.h is a good way to go in the future.

@asl
Copy link
Contributor Author

asl commented Jun 2, 2024

This should be ok, but I think the idea of unifying toString and dbprint with an additional flag in dbprint.h is a good way to go in the future.

Yes, I already opened #4698 to track this!

asl added 10 commits June 1, 2024 21:08
  - Use universal references
  - Consistently forward the packs
  - Ensure we move things passed by value
  - Use C++17 constructs
 - prefer dbprint() method for operator<<(ostream&) when present
 - otherwise, fallback to toString(), if present
@asl asl enabled auto-merge June 2, 2024 04:12
@asl asl added this pull request to the merge queue Jun 2, 2024
Merged via the queue into p4lang:main with commit 4557b06 Jun 2, 2024
18 checks passed
@asl asl deleted the string-error-helper branch June 2, 2024 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser) run-sanitizer Use this tag to run a Clang+Sanitzers CI run. run-static Use this tag to trigger static build CI run. run-ubuntu18 Use this tag to trigger a Ubuntu-18 CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants