[PATCH] D154709: [clang][ASTImporter] Add a 'Message' member to ASTImportError and use it throughout ASTImporter

Tue Jul 11 01:46:53 PDT 2023

Michael137 added a comment.

In D154709#4487776 <https://reviews.llvm.org/D154709#4487776>, @balazske wrote:

> The goal is to have a message always in `ASTImportError`? Then probably the constructor without message can be removed, at least to check if really the message is added at all places. I found that it is missing in `VisitObjCImplementationDecl`.

Agree, will do that

> I do not know what happens with messages passed to FromDiag or ToDiag and if these are available somehow to LLDB.

Ideally yes, but I found they're not very helpful in LLDB's use-case because:

1. the diagnostic doesn't get issued at all call-sites
2. LLDB switches off any `odr` related warnings because AFAIU they occur a lot (outside the ASTImporter) and would flood the user with redundant information

> Otherwise it would be even better to remove all FromDiag and ToDiag messages from ASTImporter and put these messages into ASTImportError and use later in the way that is needed by the use case. This would require to pass a message to HandleNameConflict but then we have always the detailed message. There are places where diagnostic is generated but there is no error, we should check if this is correct and if we can remove the diagnostic or make import error.

That sounds like a good way forward. The `-ast-merge` clang option does benefit from these diagnostics. So we would probably need to change it to dump the `ASTImportError` instead of relying on the diagnostics.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154709