- User Since
- Jul 12 2012, 2:19 PM (306 w, 1 d)
Thu, May 24
Rather than disabling typo correction in TransformTypos, it would be preferable to attempt to immediately correct them. (That should allow the variableX.getX() case to still work.)
Either the previous version of this patch or a version with a bool factored out seem OK to me.
Some minor nits. I would also like to see a test for the unoptimized (-O0) IR generated by Clang for the case where there are two NRVO variables in the same function. Otherwise, this looks good to go!
Wed, May 23
Thanks! This looks like exactly the right way to compute when to apply NRVO. It'd be good to track (in your commit message at least) that this addresses PR13067.
Tue, May 22
Converting to printf("single string literal") will likely be a small performance pessimization (because the string literal must be scanned for embedded % characters). I think the most efficient form is instead likely to be a format string comprising *only* format specifiers; for example, given
Mon, May 21
I've done some more investigation, and I now think this patch is merely working around deficiencies elsewhere. See https://reviews.llvm.org/D47166, which aims to fix those deficiencies more directly.
Please instead add an llvm_unreachable outside the switch, so that we still get warnings on missing switch cases if an enumerator is added.
Fri, May 18
This change causes the compiler to crash under -fno-builtin mode, presumably because it's not checking that there is a memcmp builtin library function available for use.
Hmm, perhaps our strategy for handling aligned allocation on Darwin should be revisited. We shouldn't be defining __cpp_aligned_allocation if we believe it doesn't work -- that will break code that uses aligned allocation where available and falls back to something else where it's unavailable.
Please see https://reviews.llvm.org/D47092 for the external -> weak_odr change.
Thu, May 17
As it happens, the C++ committee fixed the language wording hole here very recently. The new rule can be found here: http://wg21.link/p0968r0#2227
In summary: we should to consider the destructor for all elements of the aggregate to be potentially-invoked.
Wed, May 16
Thanks, looks great.
This is causing ASan's ODR violation detector to fire. I think the problematic case looks like this:
Tue, May 15
The deprecated enumerator is also referenced by tools/c-index-test/c-index-test.c
Looks good. It might be beneficial to do the lookup twice in this case and then issue a diagnostic if the results differ, but given how rarely this is happening, it doesn't really seem worthwhile.
Mon, May 14
This would violate our guidelines for diagnostic messages; see https://clang.llvm.org/docs/InternalsManual.html#the-format-string
Looks good, thanks.
How do you deal with the fact that node handles for map-like types need to be able to mutate a const member of a pair in-place? Are you assuming/hoping the compiler won't do bad things to the program as a result, or is there some reason why it would be unlikely to do so / disallowed from doing so? If you're just casting pair<const K, V> to / from pair<K, V> (and I think that's what's happening), there is a very real risk that struct-path-sensitive TBAA will decide that pair<const K, V>::first and pair<K, V>::first cannot alias (and likewise for ::second) and then miscompile code using libc++'s node pointers.
Clang's current behavior is observably wrong in MS compatibility mode. For example:
Also we probably want to hold off on landing this until PR37458 is fixed, otherwise std::launder will start miscompiling code.
LGTM with a fix (and test) for pointer-to-array-of-dynamic-class-type handling.
Sun, May 13
Sat, May 12
Thanks, this is great.
Fri, May 11
Justin and I looked at a few more examples, and I think we have a somewhat better understanding now. The general behavior is:
Everything old is new again. This was discussed when -fclang-abi-compat was introduced; see https://reviews.llvm.org/D36501 for the argument why this patch is the wrong way of modeling an ABI. Forcing the ClangABICompat language option as a way of "tricking" Clang into producing the PS4 ABI is a hack. The various ABI changes that -fclang-abi-compat= controls are simply part of the PS4 ABI, and that knowledge should idealistically be carried by the CodeGen (etc) code that knows about PS4, rather than by imagining that there is some other PS4 ABI that Clang would produces at version Latest, and that we're asking for a compatibility version of it.
Rather than suppressing the warning, should we instead give such variables external linkage?
Thank you, I've been wanting this for ages but never got around to it :)
Thanks, I think this can be simplified a bit but this looks like the right fix.