-
Notifications
You must be signed in to change notification settings - Fork 348
[ImportVerilog] Bump slang
#7792
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
base: main
Are you sure you want to change the base?
Changes from all commits
ba4bd79
a2228eb
fe72953
a1b6200
4b67029
033237d
be789b2
82a14cc
1d562dd
798d658
16688ca
26a05c4
069a8be
92b7025
7f2327b
19b38d9
6d86d36
a69e566
5ff9fe2
49a5ae7
7897219
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -647,8 +647,8 @@ inline FVInt operator-(uint64_t a, const FVInt &b) { | |
|
||
inline FVInt operator-(const APInt &a, const FVInt &b) { return FVInt(a) - b; } | ||
|
||
inline bool operator==(uint64_t a, const FVInt &b) { return b == a; } | ||
inline bool operator!=(uint64_t a, const FVInt &b) { return b != a; } | ||
inline bool operator==(uint64_t a, const FVInt &b) { return b.operator==(a); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How is this related to the slang version? Is it picking up some overzealous global operator in slang somewhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a consequence of building There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, just an artifact of C++20 and not anything slang is doing. Maybe you can just ifdef out these overloads when compiling in C++20 mode. |
||
inline bool operator!=(uint64_t a, const FVInt &b) { return b.operator!=(a); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any tips on how to best deal with https://timsong-cpp.github.io/cppwp/n4868/over.match#oper-3.4.4 in a codebase that still needs to support older standards? |
||
|
||
inline raw_ostream &operator<<(raw_ostream &os, const FVInt &value) { | ||
value.print(os); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually needed? slang's CMakeLists already sets these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get an error trying to build
ImportVerilog
due to public headers inslang
requiring C++ 20 if I remove this, but perhaps there's a more principled way to solve this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, that makes sense. It's a little unfortunate that this is adding friction but I make extensive use of, e.g. std::span that would be hard to remove from the public headers. Hopefully LLVM / MLIR / CIRCT can upgrade to C++20 at some point across the board; it's coming up on 5 years old.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drive by comment: in general, it's OK to require c++20 (or other non-llvm-standard things like exceptions) if they're optional and can be disabled. Since slang is an optional dependency, it's OK to require c++20 when slang is being used. We just need to be sure to run a ci build with c++17 and no slang.