- User Since
- Oct 8 2012, 9:19 AM (353 w, 3 d)
I was rather hoping for test coverage for all the new error messages this change introduced - is that unrealistic/impractical?
Tue, Jul 16
Mon, Jul 15
Test coverage? (you could add another inlined call to the test case you added for the feature - but just leave the inlined-at location with a zero column for that particular call - so you can still test both positive/negatiev in the one file)
What's the motivation for this?
Should it be conditional on "-gcolumn-info"? (or skipping it in general if the column is zero? that'd make "-gno-column-info" fall out naturally)
(we don't put decl_column on subprogram DIEs, for instance - admittedly, it's more likely the same function could have multiple inline call sites on the same line than you'd have multiple functions defined on the same line)
Looks like this code already had rudimentary error handling along this codepath where the assert is (the function returns empty, which returns false, which becomes an error further up, I think?) - and adding more descriptive/precise error handling along this path is probably good - but probably also means more testing is required to demonstrate all the new error results/messages/codepaths?
Fri, Jul 12
Thu, Jul 11
Wed, Jul 10
Tue, Jul 9
Mon, Jul 8
Looks good :)
I would've assumed these conditionals were added by Sony folks for their change in default dialect - that doesn't necessarily mean these tests are needed upstream (the functionality may be tested elsewhere)
Does this solution extend/address other label deltas, like those that would appear in the other DWARF/non-frame sections? (eg: currently we use a length in the DWARF high_pc because it means not having to use a relocation for high_pc, which reduces object size - so, does this fix cause relocations to be used for that length expression to ensure they're correct if the linker changes the instructions in that range?)
Looks good to me. (possible the check (for no default capture and no captures) could be unified with the same check/language used for the implicit function pointer conversion operator ("if (Captures.empty() && CaptureDefault == LCD_None)" - around 1744 in SemaLambda.cpp) - maybe a common utility function to test this condition or the like. But it's hardly a big thing to have it written in two places, really)
Wed, Jul 3
Tue, Jul 2
Mon, Jul 1
Did the broader design discussion about bugpoint lead to this design (a separate llvm-reduce tool)? I didn't quite follow that thread far enough to see the conclusion that lead to this direction (rather than improving/modifying bugpoint) - might be worth writing a bit more in the commit description about the conversation (link to the thread if that's helpful) and design choices - and long-term plan (to merge behavior, share things, remove one or the other - or should these two tools exist indefinitely with different goals/uses?)
Yeah, that looks incorrect/meaningless in this context - thanks for the catch!
Fri, Jun 28
Thu, Jun 27
Looks good to me, please commit
Wed, Jun 26
Sounds alright :)
Tue, Jun 25
Mon, Jun 24
My understanding was/is that iterators can't return values instead of references (that such a thing doesn't meet the standard requirements for iterators) and that iterators basically have to have internal storage for things like this.
I'm kind of thinking a change to the cursor as Greg suggested might be better than wrapping the stream further - and probably using llvm::Error.
Fri, Jun 21
Given figuring out error handling for DataExtractor is perhaps a wider issue - if you want to go ahead with this change (continue with the review & defer error handling improvements for later, leave a FIXME, etc) that seems fine.
Awesome - thanks!
Thu, Jun 20
Sounds alright to me
Wed, Jun 19
Jun 18 2019
Jun 17 2019
Is it practical/possible to do this in LLVM rather than in Clang? I'd have thought it'd be best to keep the IR metadata as output-format agnostic as practical/possible to reduce divergent codepaths, etc?
Ping (just curious about the change to the canonicalization printing policy - making sure I don't break something important)
Does the original bug show up with TSan on any existing/checked in tests, or could TSan catch it in some new test that isn't checked in yet/should be checked in?
Jun 13 2019
Jun 12 2019
I don't fully have the direction you've in mind/that this is a part of in my head - but roughly enough to give a thumbs-up here :)
Jun 11 2019
Looks good - thanks!
Jun 10 2019
Jun 7 2019
Sounds good - thanks
Jun 3 2019
May 29 2019
Can you commit this yourself - or would you like me to do it for you?
Seems good to me - thanks a bunch (:
Might be easier as a few patches - renaming the existing option, adding the new one, then removing the single split dwarf flag handling in favor of implying that by the absence of an output file name. (if I'm reading what this patch does)
May 28 2019
Generally looking pretty good to me.
Is this the change we were discussing the other week semi-related to preserved enums? It sounded to me like the conclusion was somewhat to go ahead without adding all these things to the constant list and see how it goes? Is this case different from the enum case in some way?
Any chance of a test case for this? (if you replace the null check with an assert for non-null, that might help identify a test case that hits this code?