[flang-commits] [PATCH] D129156: Add -fpass-plugin option to Flang

[Tarun Prabhu via Phabricator via flang-commits]

tarunprabhu added inline comments.


================
Comment at: flang/include/flang/Frontend/CodeGenOptions.h:49
+
 public:
   CodeGenOptions();
----------------
awarzynski wrote:
> tarunprabhu wrote:
> > awarzynski wrote:
> > > DELETEME
> > Do you want me to delete the second "public:" label?
> it is not needed, is it?
Strictly speaking, no. I tend to group data members and member functions with the same access specifier and explicitly add the access specifier for each group. If the class ever grows, it just helps to keep things a bit better organized. I didn't look at the rest of the flang code to see if it is done differently elsewhere. 


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:674
+    } else {
+      diags.Report(clang::diag::err_fe_unable_to_load_plugin)
+          << pluginFile << passPlugin.takeError();
----------------
awarzynski wrote:
> tarunprabhu wrote:
> > awarzynski wrote:
> > > IMO, it's a bit weird to report an error and to continue anyway. Apparently that's what Clang [[ https://github.com/llvm/llvm-project/blob/f80a4321ef1bafcd8041884bcb85d9ba24335adb/clang/lib/CodeGen/BackendUtil.cpp#L791-L800 | does ]] 🤔 . Do you know why?
> > I think the idea is to display as many errors as reasonably possible instead of just failing on the first error that was encountered. I imagine that the Diagnostics object was intended to be used when parsing where this approach is helpful. But when using it elsewhere, you end up with more ... peculiar behavior. 
> So, an "error" indicates that something went fundamentally wrong and indeed, `flang-new` will fail (that's why you had to use `not` in tests). If the driver does not immediately return here, could you add a comment to hint "why"? I also went over the discussion in https://reviews.llvm.org/D56935 and the rationale in Clang is still unclear to me. How about you?
> 
> If you are simply trying to reproduce the Clang semantics here, then that would be a good comment for me.
I think it's just deferring failure until later because that can be more helpful. It lets you inform the user about multiple potential problems at the same time. If you look at clang/lib/Frontend/CompilerInvocation.cpp, you will see a pattern where the `DiagnosticsEngine::getNumErrors()` is called at the start and end of each function, to indicate whether that particular functionality was completed without errors. Even there, the error is recorded, the but the failure is deferred. 

But, I haven't been involved with clang development, so I am hardly an authority on the matter and I could be completely wrong about this.

Nevertheless, I'll add a comment saying that the code is intended to mimic clang. 


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

https://reviews.llvm.org/D129156