- User Since
- Jun 10 2018, 8:27 AM (49 w, 2 d)
Wed, May 15
Wed, May 8
Address Aaron's comments.
Thu, May 2
Wed, May 1
Already done in r359702 :)
Tue, Apr 30
Mon, Apr 29
@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 ?
Thu, Apr 25
Tue, Apr 23
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.
Mon, Apr 22
Sun, Apr 21
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.
Apr 21 2019
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!
Apr 8 2019
After looking at the bug report (https://bugs.debian.org/877359), I would like to point out that there is currently *absolutely* no stability guarantee for the serialization format (ie, you need the exact same revision). In regard of this I am wondering how sane it is to ship pch files.
Apr 2 2019
Mar 31 2019
Mar 30 2019
It seems that the tests are not present in this diff ? Also, again, could you please:
- Use clang-format, and
- Make sure that the comments are full sentences with appropriate punctuation, and
- Follow the style guide regarding for the names of variables and functions.
+1. I wanted to do this but never bothered. Did you go over all the statements on Stmt.h systematically ? You could also do the same thing for all of the statements/expressions in StmtCXX.h, Expr.h, ExprCXX.h, and so on. Apart from this did you run clang-format on the patch ?
Mar 29 2019
Mar 25 2019
Mar 24 2019
Great! Since this already received one round of reviews I guess this looks okay.
Mar 23 2019
Before any further review, could you please run clang-format on your patch (but not necessarily on the tests and not on the *.td files), wrap lines to 80 cols, and in general use complete sentences in comments (that is with proper punctuation and capitalization) ? To run clang-format on your patch you can use clang-format-diff. This is the command I use :
Mar 22 2019
To expand on the above, if you need to add a few bits to a statement/expression you can use the bit-field classes in Stmt (for example: IfStmtBitfields). You will also need to update the serialization code in Serialization/ASTWriterStmt.cpp and Serialization/ASTReaderStmt.cpp.