zyounan added a subscriber: rsmith.
zyounan added a comment.

Thanks for the reply. And I understand (and agree with) the point that it's 
better to solve this problem once and for all.

> Currently the code in CompilerInstance::ExecuteAction seems to acknowledge 
> that there should be a fallback. I'm suggesting to move this fallback down to 
> a function that actually runs parsing.

One thing I'm afraid of is, that there are/were some compatible reasons that 
made us decide not to insert this call at `ASTFrontendAction::ExecuteAction` 
nor `clang::ParseAST`. (I didn't see the explanation in D66361 
<https://reviews.llvm.org/D66361>. Richard, could you kindly explain why? 
@rsmith)

> That's `CompilerInstance::ExecuteAction`

Sorry for my mistake. I thought placing that in `CompilerInstance` was enough 
for most tools since it's less likely for developers to invoke the 
FrontendAction on their own, and for "advanced" users (like clangd) who'd like 
to control every step handling the AST, maybe it's fine to give them the 
opportunity to decide if it should crash on infinite recursion?

However, creating such a discrepancy doesn't make much sense. I'm happy to move 
this call to libclang if this won't break many things.

> Could you clarify what kind of encapsulation would it break?

(That's just my spitballing, please don't mind. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158967

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to