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

Replace boost string algorithms with their abseil counterparts #4482

Merged
merged 11 commits into from
Mar 5, 2024

Conversation

asl
Copy link
Contributor

@asl asl commented Feb 28, 2024

Some refactoring and cleanup while there:

  • Code moved from headers down to cpp
  • StringRef replaced with std::string_view (header is not removed as I do not know how this might affect downstream)

@asl asl requested review from grg, vlstill and fruffy February 28, 2024 08:45
@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Feb 28, 2024
@asl asl force-pushed the abseil-str branch 2 times, most recently from 4e379ee to 072b827 Compare February 28, 2024 21:18
@asl asl requested a review from fruffy February 28, 2024 21:35
Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

Thank you for cleaning away the StringRef. Could you also put [[deprecated(...)]] on the StringRef struct so the downstream tools are kind reminded to upgrade and we have a path to drop it completely some time in future? I think that would make sense :-).

On a more general note: is there a strong reason to replace boost algos with their abseil counterparts? It abseil significantly better? Is it easier to use? Do we strive to get rid of boost altogether? Since we have abseil for other reasons, I am not against this, but for example in case of the substitutions the boost syntax had some advantages (named paceholders).

frontends/parsers/parserDriver.cpp Outdated Show resolved Hide resolved
lib/cstring.h Outdated Show resolved Hide resolved
@@ -319,6 +319,7 @@ class InputSources final {

} // namespace Util

// FIXME: Do we really need to have this in header? This pulls the whole iostream header
inline void dbprint(const IHasDbPrint *o) { o->dbprint(std::cout); }
Copy link
Contributor

Choose a reason for hiding this comment

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

At the very least, we can probably put the implementation in .cpp. But this seems like maybe it is there only to simplify debugging with gdb.

@asl
Copy link
Contributor Author

asl commented Feb 29, 2024

On a more general note: is there a strong reason to replace boost algos with their abseil counterparts? It abseil significantly better? Is it easier to use? Do we strive to get rid of boost altogether? Since we have abseil for other reasons, I am not against this, but for example in case of the substitutions the boost syntax had some advantages (named paceholders).

I like abseil string algorithms more. For example StrSplit could return a vector of string_view's, essentially skipping the copies of string pieces. It also returns vector by value, boost::split takes output by reference.

@fruffy
Copy link
Collaborator

fruffy commented Feb 29, 2024

On a more general note: is there a strong reason to replace boost algos with their abseil counterparts? It abseil significantly better? Is it easier to use? Do we strive to get rid of boost altogether? Since we have abseil for other reasons, I am not against this, but for example in case of the substitutions the boost syntax had some advantages (named paceholders).

My personal opinion is that it is nice to keep our dependencies simple. Since we are basically forced to use Abseil with Protobuf we might as well make use of it. Since we do not want to keep two utility frameworks around we should slowly deprecated widespread usage of boost. Another reason I am in favor of slowly phasing out boost is that is generally a huge framework and unlike Abseil including one boost header can pull in many others. We do not need to get rid of it entirely but should be judicious of what we include. #3898 is an attempt to simplify things a little.

@asl
Copy link
Contributor Author

asl commented Feb 29, 2024

My personal opinion is that it is nice to keep our dependencies simple. Since we are basically forced to use Abseil with Protobuf we might as well make use of it. Since we do not want to keep two utility frameworks around we should slowly deprecated widespread usage of boost. Another reason I am in favor of slowly phasing out boost is that is generally a huge framework and unlike Abseil including one boost header can pull in many others. We do not need to get rid of it entirely but should be judicious of what we include. #3898 is an attempt to simplify things a little.

I think after this PR we'll end with:

  • few containers here and there, that could be replaced with some header-only implementation dropped to lib.
  • boost::graph for graph backend
  • boost::multiprecision
  • boost::random
  • boost::format

The latter could be replaced by fmt or Abseil's format routines, or we can wait for next standard bump. It's just huge pile of work :)

boost::random is used for Mersenne-Twister – need to double check if these days normal standard library is mature enough to instantiate rng with bigint type.

@vlstill
Copy link
Contributor

vlstill commented Mar 1, 2024

boost::random is used for Mersenne-Twister – need to double check if these days normal standard library is mature enough to instantiate rng with bigint type.

It seems it is not :-/

UIntType -- The result type generated by the generator. The effect is undefined if this is not one of unsigned short, unsigned int, unsigned long, or unsigned long long. (https://en.cppreference.com/w/cpp/numeric/random/mersenne_twister_engine)

Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@asl
Copy link
Contributor Author

asl commented Mar 5, 2024

@fruffy Will you please take a look when you have a chance? Thanks!

@fruffy
Copy link
Collaborator

fruffy commented Mar 5, 2024

@fruffy Will you please take a look when you have a chance? Thanks!

Will try to give a look tomorrow, things have been a bit busy lately..

@asl
Copy link
Contributor Author

asl commented Mar 5, 2024

Will try to give a look tomorrow, things have been a bit busy lately..

Sure, no worries! Just wanted to clear my patch queue a bit :)

frontends/p4/typeChecking/typeConstraints.cpp Show resolved Hide resolved
frontends/parsers/v1/v1lexer.ll Show resolved Hide resolved
lib/source_file.h Outdated Show resolved Hide resolved
lib/cstring.h Show resolved Hide resolved
lib/source_file.cpp Outdated Show resolved Hide resolved
@asl asl added this pull request to the merge queue Mar 5, 2024
Merged via the queue into p4lang:main with commit d8c3f42 Mar 5, 2024
17 checks passed
@asl asl deleted the abseil-str branch March 5, 2024 22:19
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants