User Details
- User Since
- Jul 15 2015, 7:49 AM (363 w, 6 d)
Mon, Jun 13
Tue, Jun 7
LGTM but please get a second approval before submitting
LGTM but please get a second approval before submitting
LGTM but please get a second approval before submitting.
Mon, Jun 6
Would it be possible to test this as-is, or are other changes necessary to meaningfully exercise this code?
LGTM
LGTM
Jun 5 2022
LGTM
Jun 4 2022
Jun 1 2022
Kind ping
LGTM
May 27 2022
May 25 2022
Ping
LGTM but please get a second approval before submitting.
May 24 2022
May 19 2022
On high level, would it be possible to use an existing class like llvm::BinaryStreamReader instead that most of this data reading machinery?
May 16 2022
May 10 2022
LGTM but please get another one before submitting
May 9 2022
LGTM but please get another one before submitting.
May 8 2022
Apr 25 2022
LGTM modulo nits. Thanks for the fixes!
Apr 11 2022
+1, I had to patch this locally in my project for exactly the reasons you mentioned
Apr 8 2022
LGTM
Apr 7 2022
Looks good to me overall, I just left some local comments. Please take my writing suggestions with a pinch of salt, English is my second language.
Mar 29 2022
LGTM, thanks for the fixes.
Could you also add some tests that make sure this can be constant-evaluated? (e.g., with static_assert)
Mar 25 2022
Mar 22 2022
Mar 21 2022
A few thoughts:
- Is the requirement for this to be constant folded by the optimizer?
- This could be made constexpr if we wanted.
- The function name suggests this handles enums only but it works with any copybale & comparable type. I think we should either restrict it to enum types or make the name more general.
- Since this function is not constexpr now, how about we added an overload to is_contained accepting an initializer list as the second element, e.g.: is_constained(MyEnum::A, {MyEnum::A, MyEnum::B, MyEnum::C})?
Mar 16 2022
I'm not too sure about the semantics either. I think it might be easier to think about if we tie this back to regular graphs without multi-edges.
Mar 15 2022
Feb 23 2022
LGTM but please get at least one additional approval before submitting
Feb 14 2022
Feb 10 2022
Thanks for working on this, @xbolva00! I don't know this part of the codebase, so won't comment on the patch itself. Just a few questions/suggestions:
Jan 30 2022
LGTM
Jan 28 2022
Jan 5 2022
LGTM
Jan 4 2022
This change is NFC but can improve the runtime of the compiler dramatically in some pathological cases (where the pass was pushing a lot (several thousands) of small updates (less than 6)).
For instance on the motivating example we went from 300+ sec to less than a second.
Dec 23 2021
Dec 18 2021
Dec 16 2021
Nov 26 2021
I think it would be good to send the DomTree documentation changes separately as they are a clear improvement regardless of whether this patch goes in or not.