[PATCH] D62373: [ASTImporter] Store import errors for Decls

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.


================
Comment at: clang/lib/AST/ASTImporter.cpp:7851
+    if (!getImportDeclErrorIfAny(FromD)) {
+      // Error encountered for the first time.
+      // After takeError the error is not usable any more in ToDOrErr.
----------------
martong wrote:
> balazske wrote:
> > martong wrote:
> > > a_sidorin wrote:
> > > > Is it possible to get this error more than once?
> > > Yes, that can happen in cyclic imports like: ClassTemplateDecl -> TemplatedDecl -> ClassTemplateDecl.
> > I do not understand this completely, in this branch `setImportDeclError` is called so at next time we can not go into this branch again (for the same `FromD`). (We can get the error and not go into this branch more than once but get no error and go into the branch only once.)
> Yes, I missed that.
> So, perhaps we can remove the condition `if (!getImportDeclErrorIfAny(FromD))` ?
I think yes: If `!ToDOrErr` is true we have an error that was newly created in `ImportImpl`. But I am not totally sure, and the ImportImpl can be overridden, so it is better to not remove the condition. Or make an assert out of it: `assert(!getImportDeclErrorIfAny(FromD) && "Import error already set for decl.")`.


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:4697
+                        DefaultTestValuesForRunOptions, );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
----------------
martong wrote:
> balazske wrote:
> > a_sidorin wrote:
> > > #undef ERRONEOUSSTMT?
> > Maybe we should not use a macro for this, specially not define the macro inside the `struct` block. For the current tests it is sufficient to make a std::string that contains something like `void f() { asm(""); }`.
> My first attempt was to use std::string for the errouneous Stmt and then to use llvm::formatv to easily concatenate the test code examples.
> However, it tunred out that formatv actively forbids the use of the braces in the source string.
> So I decided to use a macro because that way we can build up the test code most easily.
> What is the problem by defining the macro in a struct block? There are no blocks for the preprocessor, just directives. I'd like to indicate that the macro is used only by the test.
The problem was that it looks like that macro is an internal thing (to the struct) but in reality it is not. Probably `std::string::append` can be used if possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62373