Page MenuHomePhabricator

[flang][driver] NFC: Make code more in line with LLVM style
ClosedPublic

Authored by awarzynski on Apr 22 2022, 4:08 AM.

Details

Summary

This patch basically implements [1] in ExecuteCompilerInvocation.cpp. It
also:

  • replaces CreateFrontendBaseAction with CreateFrontendAction (only one method is needed ATM, this change removes the extra indirection)
  • removes InvalidAction from the ActionKind enum (I don't think it adds much and keeping it would mean adding a new void case in CreateFrontendAction)

No new functionality is added, hence no tests.

[1] https://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations

Diff Detail

Event Timeline

awarzynski created this revision.Apr 22 2022, 4:08 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 22 2022, 4:08 AM
awarzynski requested review of this revision.Apr 22 2022, 4:08 AM
This revision is now accepted and ready to land.Apr 22 2022, 4:36 AM
ekieri accepted this revision.Apr 23 2022, 12:00 AM

Thanks for taking the time to work on quality and compliance! LGTM, but please consider my nits.

The removal of InvalidAction made me worry about the default. If I understood correctly, the default programAction is set (safely) here. But would it not feel more secure to set it directly in the definition instead? Anyway, makes sense to me to remove InvalidAction. A sensible default is more helpful than a failing default.

flang/include/flang/FrontendTool/Utils.h
19–20

Then this forward declaration should go as well.

And please remove the #include from FrontendAction.cpp. An unrelated redundancy, but somewhat related :)

Address Emil's nits.

Thanks for taking a look, @ekieri !

The removal of InvalidAction made me worry about the default. If I understood correctly, the default programAction is set (safely) here. But would it not feel more secure to set it directly in the definition instead? Anyway, makes sense to me to remove InvalidAction. A sensible default is more helpful than a failing default.

These are good points, thanks! I would still like to set programAction in ParseFrontendArg (on top of having a default in FrontendOptions.h) - that's where other parts of CompilerInvocation are set. Also, this way the defaults in FrontendOptions.h don't matter as long as they are sane. WDYT?

Thanks for addressing my comments!

These are good points, thanks! I would still like to set programAction in ParseFrontendArg (on top of having a default in FrontendOptions.h) - that's where other parts of CompilerInvocation are set. Also, this way the defaults in FrontendOptions.h don't matter as long as they are sane. WDYT?

Sure, those are good arguments, I have no problem with this solution.