ChuanqiXu marked 10 inline comments as done.
ChuanqiXu added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:2762
         SawDefaultArgument = true;
-        RedundantDefaultArg = true;
+        if (!OldTypeParm->getImportedOwningModule())
+          RedundantDefaultArg = true;
----------------
rsmith wrote:
> This is the wrong thing to check -- whether the owning module is imported 
> (from an AST file) is irrelevant. We might parse multiple translation units 
> in the same compilation, and we might split a single translation unit into 
> multiple compilations (eg with a precompiled preamble) resulting in some of 
> the same translation unit being in an AST file. What matters is whether the 
> declaration is in the current translation unit or a different translation 
> unit.
> 
> I'm not sure what the best way to detect that is currently. You might be able 
> to just compare `getOwningModule()` of the two declarations, but I don't 
> think that will properly handle cases in the global module fragment, or cases 
> where one template is in the primary module interface unit and another is in 
> a module implementation unit of the same module.
> 
> Ideally I would like for us to create one `TranslationUnitDecl` per 
> translation unit, rather than shoving all imported code into the translation 
> unit of the importer, but that's a moderately large change, which I'd like to 
> not block this change on if we can avoid it. But I think that's the right 
> longer-term approach. Then here we would just need to find the 
> lexically-enclosing `TranslationUnitDecl` for each template parameter and see 
> if they're the same.
I found we could determine whether the declaration is in the current TU or not 
by using `Sema::ModuleScopes`, which contains the modules we're currently 
parsing.


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

https://reviews.llvm.org/D118034

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to