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:
> > 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.")`.
Ok, I changed to have an assertion.
================
Comment at: clang/unittests/AST/ASTImporterTest.cpp:4697
+ DefaultTestValuesForRunOptions, );
+
INSTANTIATE_TEST_CASE_P(ParameterizedTests, ASTImporterLookupTableTest,
----------------
balazske wrote:
> 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.
Ok, I removed the macro and use rather std::string.operator+()
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62373/new/
https://reviews.llvm.org/D62373
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
