- User Since
- Jun 10 2018, 8:27 AM (67 w, 20 h)
Tue, Aug 27
Sun, Aug 25
Also test that DeclInfo is trivially destructible.
I was looking at SpecificBumpPtrAllocator, which knows it's type.
But if i look down the indirection, i think AllocatorBase::Allocate<T>()/AllocatorBase::Deallocate<T>() is the place.
Aug 23 2019
Aug 22 2019
This should not warn. Please verify that patch was applied correctly and you use newly built clang with this patch. (I use arc patch DXXXXX)
Aug 21 2019
Is the test up-to-date ? I tried to apply the patch locally and I get a bunch of failures :
Aug 20 2019
Can you submit the patch with the full context (ie: git diff -U9999999) ?
Rebased. Thanks for the review @xbolva00 !
Aug 13 2019
I think that you have to run check-clang-tools. I too was surprised that it was not included in check-clang.
Aug 12 2019
It seems that these two options are not exactly the same right ? The ContainsError bit is useful to quickly answer "Does this expression contains an invalid sub-expression" without doing the search, while adding an ErrorExpr node is useful to note that this sub-expression is invalid (and as Aaron says the hypothetical ErrorExpr node can carry more info about the error).
Jul 27 2019
May 15 2019
May 8 2019
Address Aaron's comments.
May 2 2019
May 1 2019
Already done in r359702 :)
Apr 30 2019
Apr 29 2019
@rsmith Do you have an opinion on whether this ADL rule should be implemented ?
Is it intentional to warn even if all the cases are covered ?
Since this is target-specific, is it right to put this here ?
Apr 25 2019
Apr 23 2019
Closing this since the issue was fixed in r358674.
Added a test which exposes a new problem that this patch incidentally solves (see N_conflicting_namespace_alias). Because of the using directive using namespace M;, the namespace alias i for the namespace Q is found in the redeclaration lookup. Before this patch, since getAsSingle used internally getUnderlyingDecl(), PrevDecl was for the NamespaceDecl for Q, and not for the VarDecl for Q::i. Then the declaration for the enumerator i was mistakenly rejected since Q is in the same scope.
- Added a test (see N_shadow) for the behavior of Wshadow. This test showed that I forgot to change CheckShadow(New, PrevDecl, R); to CheckShadow(New, PrevDecl->getUnderlyingDecl(), R); to match change in the condition of the if statement.
Apr 22 2019
Apr 21 2019
I will take a look at this tomorrow, I know that it is annoying to get no feedback!
@Quuxplusone Do you have other objections apart from the template-id issue ? If not, since D60573 depends on this patch, I would like to commit this with a comment explaining the issue instead of the FIXME.
This looks reasonable to me (although I think that the test should be in SemaCXX/ and not in Parser/, but the test for the non-template case is already in Parser/ so ok).
Apr 20 2019
This patch certainly took a few hours to write. Can you spend 5 minutes on making the summary readable (spelling, capitalization, formatting, well-formed sentences, ...) ?
A few comments/questions:
Apr 18 2019
Apr 17 2019
By the way, I am wondering about how much this is tested. I did look quickly in test/PCH and it appears that there are only 3 (short) tests : ocl_types.cl, opencl-extensions.cl and no-validate-pch.
What about something like D60835 ?
Maybe, but is there an equivalent to getTypeID for declarations ?
Mmm. I hope I am not missing something obvious, but how is this actually fixing the issue ? I don't see why the order between the Type * and between the Decl * should be deterministic (I think they will be when they are part of the same slab in the allocator, but I don't see why the slab ordering should be stable).
@rsmith @aaron.ballman Do you have any opinion on whether the ADL rules from CWG 997 should be implemented ? The issue here as I understand it is that these rules expand ADL, but no one apart from GCC implement them. By implementing these rules, the argument to remove them in the future becomes weaker since real code might actually start to depend on them.
Apr 15 2019
This is also very useful to test that a given warning is only emitted in c++xx. Funnily when grepping for verify= in test/ most matches are from tests exercising this functionality.
For information we are discussing in D60570 whether it is actually a good idea to implement this rule.
(I have numbered your examples to make the discussion easier, and moved to a non-inline comment to have more space)
Apr 14 2019
Also test that typedef-names and using-declarations are not considered.
Apr 13 2019
Apr 12 2019
Sorry for the delay. Thanks a lot for the patch!
Apr 11 2019
Removed the call to isTransparentContext() in the loop in CollectEnclosingNamespace. It is not actually needed since we only care about finding the innermost enclosing namespace.
Also test for inline namespaces.
I am not the best person to review this, sorry!