- User Since
- Oct 31 2016, 11:13 AM (102 w, 1 d)
Sun, Oct 14
I am in contact with the guy that actually discovered this breakage. It seems to be a weird and hard to reproduce error, but happens on x86 as well. So it is a compile/header something combination that results in the error. I am pretty sure that will take a while until we figured it out :/
Sat, Oct 13
Unfortunatly this check does not compile on some ARM platforms. It seems that a matcher exceeds the recursion limit in template instantations.
Fri, Oct 12
Committed with https://reviews.llvm.org/rCTE344374
The patch does not apply clean, could you please take a look at it?
- make the logic with variadic clear
- remove unused enum in header file, no idea what i intended to do with it :D
- add tests and adjust doc
- one more test case
- fix headline in doc
Thousands? After the query optimization the max was 173, and that only for a single function. The next number was 64.
Unfortunately, I won't be able to review the code in the upcoming few weeks as I caught a cold and I'm trying to get better before the LLVM Meeting, so if there is anyone else interested in reviewing proposed changes please feel free to jump in.
I see, probably not worth it. Its one-time effort anyway right?
Did you verify it actually works?
Otherwise LGTM and because its a bug-fix you can commit and other concerns can be done post-commit.
This looks reasonable to me but could you please explain a bit what the issue was in the bugreport? So a very deep hierarchy causing the problem makes sense, but why was "ignoring first" the difference maker?
Would it make sense to add a warning in the documentation that this check can be very expensive? And are there measures to speed the process up somehow?
Thu, Oct 11
LG in principle, just the SmallVec thing could be done if you agree. I don't insist on it, but it looks like a performance benefit to me.
Wed, Oct 10
Tue, Oct 9
welcome to the LLVM community and thank you very much for working on that check!
If you have any questions or other issues don't hesitate to ask ;)
Mon, Oct 8
The change looks good in principle. I think it would make sense to migrate one test already, to use the new capability and check if everything works as expected. The current tests still run fine?
Sun, Oct 7
- address review comments, simplifying code
@kbobyrev is it ok for you if I stick with the Optional<> style in the check?
I think you can commit, there was enough opportunity to respond and we pinged directly as well.
Sat, Oct 6
Fri, Oct 5
C.131 seems to imply a minimal amount of trivial getters/setters before
Commited in https://reviews.llvm.org/rL343848.
Thank you for the patch!
@aaron.ballman I have a question for an expert: How would bitfields relate to this check? Can there be a similar pattern for them and do they need to be handled here?
This patch does not fix the underlying issue. I do have a candidate that might cause it, but I am not sure if it is functionally preserving (tests seems to work though!). I think it is best if @baloghadamsoftware takes a look at it as well!
Thu, Oct 4
thanks for working on this! I have one question: Why are variables _not_ considered in the check but only constants? IMHO it would make sense to transform these as well.
from my side is nothing outstanding
Committed on your behalf
This should definitly be backported, as exception-escape is in 7.0 and it hangs up on no-except builds
Thanks for clarification :)