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

[Gabor Marton via Phabricator via cfe-commits]

martong marked 4 inline comments as done.
martong 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.
----------------
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))` ?


================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:4697
+                        DefaultTestValuesForRunOptions, );
+
 INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62373