[PATCH] D118034: [C++20] [Modules] Don't complain about duplicated default template argument across modules

[Nathan Sidwell via Phabricator via cfe-commits]

urnathan added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:2681-2697
+    // Variable used to not diagnose redundant default arguments
+    // from modules.
+    //
+    // [basic.def.odr]/13:
+    //     There can be more than one definition of a
+    //     ...
+    //     default template argument
----------------
while I do approve of good commenting, this is a little wordy and I think misleading.  It is only from global module fragments that you can get duplicate declarations and those must match default parameters (I think).  so perhaps 'IsGlobalModule'?  or somehting like that.

Perhaps all of this comment shoould be at the point of checking?  After all the preceding comment mentions 'variables used to ...'?



================
Comment at: clang/lib/Sema/SemaTemplate.cpp:2852
 
-    if (RedundantDefaultArg) {
+    if (RedundantDefaultArg && !IsOldParamFromModule) {
       // C++ [temp.param]p12:
----------------
Yes, I think this is a better place for the comment.  If there are duplicates we need to check they are the same -- pedantically the same tokens, but at least AST equivalence?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118034/new/

https://reviews.llvm.org/D118034