- User Since
- Jul 12 2012, 2:19 PM (282 w, 5 d)
Mon, Dec 11
Fri, Dec 8
Huh, I'm amazed we've had this bug for so long. Thanks!
LGTM with the PS4 comment removed. Thank you!
Thu, Dec 7
I've not done a detailed review of the string manipulation here, but this looks like a great feature, thanks!
EDG and MSVC do not appear to treat this as a defect resolution; I suspect this is an oversight in GCC rather than an intentional extension. Let's convert the error to an (off by default) pedantic Extension (ISO C++11 does not allow ...), and suppress the extension warning in C++17 onwards.
LGTM, but I'd like the old IdentifierTable constructor to be removed if there are no callers left.
I think the LateParsed attributes are not going to work properly (because they'd be written in places where the function parameters are not in scope), so I wonder if we should avoid allowing them with [[clang::]] notation for now so we can choose to make them function type attributes in the future, but I'm happy with you making that call.
LGTM, makes sense to add this for GCC compatibility even if the standard ends up being called C18.
Please update the documentation and the release notes.
Wed, Dec 6
Tue, Dec 5
Mon, Dec 4
I expect there are more places that need to be changed, but this is looking surprisingly clean.
Fri, Dec 1
The intent is to use a FilenameID of -1 to represent this situation; see the documentation of the LineEntry::FilenameID member. Users of that field are expected to deal with that value (see the handling of that case in SourceManager::getPresumedLoc for example). I think we need to handle it that way, rather than mapping to the main file name as this patch does, for correctness in the PCH case -- we don't actually *know* the correct main file name at the point where the line directive appears.
We already have mechanisms to hash the AST. I'm strongly opposed to adding another one (and requiring AST modifications to update yet more such mechanisms).
Thu, Nov 30
Per https://gcc.gnu.org/onlinedocs/gcc/Floating-Types.html, this doesn't appear to be correct: _Float128 is only *sometimes* the same type as __float128.
Wed, Nov 29
What actual problem is this solving? These IR type names exist only for the convenience of humans reading the IR, and making them long (potentially extremely long) by including template arguments seems likely to hinder that more than it helps.
Thanks for your patience, this turned out to be surprisingly complicated.
Tue, Nov 28
Please add tests for the member pointer cases across various different targets.
Mon, Nov 27
Mon, Nov 20
Thanks, looks great.
Looks fine to me, but I think the alignment numbers in your summary are off by a factor of two.
Thu, Nov 16
Tue, Nov 14
Let's keep the complete review thread together on https://reviews.llvm.org/D5767.
Mon, Nov 13
Nov 11 2017
Nov 8 2017
I'm not entirely sure what's happening with this and https://reviews.llvm.org/D38818, but the direction looks good to me, and I left a couple of comments on the other review thread.
Thank you! Please move the tests to a new subdirectory under test, say test/Templight.
Nov 3 2017
I'm still unconvinced that this mechanism is worth supporting.
Nov 2 2017
Nov 1 2017
Thanks, this looks like a nice cleanup. I wonder of some of the switches over CPUKind (setting default features and macros) could also be moved into the .def file, but let's get this landed first and then see if that looks like an improvement.