[Lldb-commits] [PATCH] D68326: [lldb][modern-type-lookup] No longer import temporary declarations into the persistent AST

[Shafik Yaghmour via Phabricator via lldb-commits]

shafik added inline comments.


================
Comment at: clang/include/clang/AST/ExternalASTMerger.h:95
   public:
-    ImporterSource(ASTContext &_AST, FileManager &_FM, const OriginMap &_OM)
-        : AST(_AST), FM(_FM), OM(_OM) {}
+    ImporterSource(ASTContext &_AST, FileManager &_FM, const OriginMap &_OM,
+                   bool _Temporary = false, ExternalASTMerger *Merger = nullptr)
----------------
Identifiers the begin with an underscore and followed by a capital letter are reserved see [lex.name/p3.1](http://eel.is/c++draft/lex.name#3.1):

>Each identifier that contains a double underscore __ or begins with an underscore followed by an uppercase letter is reserved to the implementation for any use.


================
Comment at: clang/lib/AST/ExternalASTMerger.cpp:141
+    // that doesn't cause having minimally imported declarations in the target
+    // ASTContext that no connected ASTImporter has imported (and can complete).
+    //
----------------
martong wrote:
> This line/sentence is hard to parse for me.
> I get this part: 
> 
> ```
> This way the ExternalASTMerger can safely do a minimal import that doesn't cause having minimally imported declarations in the target ASTContext.
> ```
> 
> But this I don't: 
> ```
> that no connected ASTImporter has imported.
> ```
I agree this is hard to parse, although I am happy that you are explaining the rationale.


================
Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:183
 
-namespace {
-/// This class walks an AST and ensures that all DeclContexts defined inside the
----------------
So we are removing this b/c we are now doing a minimal import from the temporary source and then in the next patch you will change that?


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D68326