Flang's CompilerInvocation.cpp should not copy->paste->edit from Clang's CompilerInvocation.cpp

[jeffhammond]

Consider parseShowColorsArgs from flang/lib/Frontend/CompilerInvocation.cpp and clang/lib/Frontend/CompilerInvocation.cpp. The function has been copied almost verbatim except:

• Flang doesn't rely on using namespace llvm::opt;
• whitespace
• showColors and value follows a different case convention
• variable named opt instead of O
• default argument different, comment about it

Using copy->paste->edit like this tedious and error-prone. Any bugs in the Clang driver code will need to be fixed manually a second time in Flang. Due to the syntax changes that have no impact on code generation, this won't be as trivial because the patch to Flang will have to use the new variable names.

This code should be properly abstracted and reused.

Furthermore, Flang should follow https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly.

< static bool parseShowColorsArgs(const llvm::opt::ArgList &args,
<                                 bool defaultColor = true) {
<   // Color diagnostics default to auto ("on" if terminal supports) in the
<   // compiler driver `flang-new` but default to off in the frontend driver
<   // `flang-new -fc1`, needing an explicit OPT_fdiagnostics_color.
---
> static bool parseShowColorsArgs(const ArgList &Args, bool DefaultColor) {
>   // Color diagnostics default to auto ("on" if terminal supports) in the driver
>   // but default to off in cc1, needing an explicit OPT_fdiagnostics_color.
12,27c10,24
<   } showColors = defaultColor ? Colors_Auto : Colors_Off;
<
<   for (auto *a : args) {
<     const llvm::opt::Option &opt = a->getOption();
<     if (opt.matches(clang::driver::options::OPT_fcolor_diagnostics)) {
<       showColors = Colors_On;
<     } else if (opt.matches(clang::driver::options::OPT_fno_color_diagnostics)) {
<       showColors = Colors_Off;
<     } else if (opt.matches(clang::driver::options::OPT_fdiagnostics_color_EQ)) {
<       llvm::StringRef value(a->getValue());
<       if (value == "always")
<         showColors = Colors_On;
<       else if (value == "never")
<         showColors = Colors_Off;
<       else if (value == "auto")
<         showColors = Colors_Auto;
---
>   } ShowColors = DefaultColor ? Colors_Auto : Colors_Off;
>   for (auto *A : Args) {
>     const Option &O = A->getOption();
>     if (O.matches(options::OPT_fcolor_diagnostics)) {
>       ShowColors = Colors_On;
>     } else if (O.matches(options::OPT_fno_color_diagnostics)) {
>       ShowColors = Colors_Off;
>     } else if (O.matches(options::OPT_fdiagnostics_color_EQ)) {
>       StringRef Value(A->getValue());
>       if (Value == "always")
>         ShowColors = Colors_On;
>       else if (Value == "never")
>         ShowColors = Colors_Off;
>       else if (Value == "auto")
>         ShowColors = Colors_Auto;
30,32c27,28
<
<   return showColors == Colors_On ||
<          (showColors == Colors_Auto &&
---
>   return ShowColors == Colors_On ||
>          (ShowColors == Colors_Auto &&

[llvmbot]

@llvm/issue-subscribers-flang-driver

Author: Jeff Hammond (jeffhammond)

Consider `parseShowColorsArgs` from flang/lib/Frontend/CompilerInvocation.cpp and clang/lib/Frontend/CompilerInvocation.cpp. The function has been copied almost verbatim except: - Flang doesn't rely on `using namespace llvm::opt;` - whitespace - `showColors` and `value` follows a different case convention - variable named `opt` instead of `O` - default argument different, comment about it

Using copy->paste->edit like this tedious and error-prone. Any bugs in the Clang driver code will need to be fixed manually a second time in Flang. Due to the syntax changes that have no impact on code generation, this won't be as trivial because the patch to Flang will have to use the new variable names.

This code should be properly abstracted and reused.

Furthermore, Flang should follow https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly.

< static bool parseShowColorsArgs(const llvm::opt::ArgList &args,
<                                 bool defaultColor = true) {
<   // Color diagnostics default to auto ("on" if terminal supports) in the
<   // compiler driver `flang-new` but default to off in the frontend driver
<   // `flang-new -fc1`, needing an explicit OPT_fdiagnostics_color.
---
> static bool parseShowColorsArgs(const ArgList &Args, bool DefaultColor) {
>   // Color diagnostics default to auto ("on" if terminal supports) in the driver
>   // but default to off in cc1, needing an explicit OPT_fdiagnostics_color.
12,27c10,24
<   } showColors = defaultColor ? Colors_Auto : Colors_Off;
<
<   for (auto *a : args) {
<     const llvm::opt::Option &opt = a->getOption();
<     if (opt.matches(clang::driver::options::OPT_fcolor_diagnostics)) {
<       showColors = Colors_On;
<     } else if (opt.matches(clang::driver::options::OPT_fno_color_diagnostics)) {
<       showColors = Colors_Off;
<     } else if (opt.matches(clang::driver::options::OPT_fdiagnostics_color_EQ)) {
<       llvm::StringRef value(a->getValue());
<       if (value == "always")
<         showColors = Colors_On;
<       else if (value == "never")
<         showColors = Colors_Off;
<       else if (value == "auto")
<         showColors = Colors_Auto;
---
>   } ShowColors = DefaultColor ? Colors_Auto : Colors_Off;
>   for (auto *A : Args) {
>     const Option &O = A->getOption();
>     if (O.matches(options::OPT_fcolor_diagnostics)) {
>       ShowColors = Colors_On;
>     } else if (O.matches(options::OPT_fno_color_diagnostics)) {
>       ShowColors = Colors_Off;
>     } else if (O.matches(options::OPT_fdiagnostics_color_EQ)) {
>       StringRef Value(A->getValue());
>       if (Value == "always")
>         ShowColors = Colors_On;
>       else if (Value == "never")
>         ShowColors = Colors_Off;
>       else if (Value == "auto")
>         ShowColors = Colors_Auto;
30,32c27,28
<
<   return showColors == Colors_On ||
<          (showColors == Colors_Auto &&
---
>   return ShowColors == Colors_On ||
>          (ShowColors == Colors_Auto &&

[jeffhammond]

Apparently, the code style divergence is intentional (https://reviews.llvm.org/D125007). I don't know what to do here. Do I follow the LLVM style guide in Flang or not?

[banach-space]

Thanks for reporting this!

Consider parseShowColorsArgs from flang/lib/Frontend/CompilerInvocation.cpp and clang/lib/Frontend/CompilerInvocation.cpp

Do you have more examples? The problem with this is that there's no good project (e.g. a library) to share these hooks. And there's little value in creating a dedicated library only for 1 function. If there's more then that's a different matter. However, in many cases there are subtle differences between these functions (often coming from the fact that Flang has only limited support for many of these things).

Also, there's a bigger issue of extracting the Driver code out of Clang and moving it to a dedicated lib. We were hoping that there would be more progress on that, but it has never materialised.

Finally, "CompilerInvocation.cpp" implements the frontend driver. Flang and Clang share the code for the compiler driver, but not the frontend driver. I agree that the duplication is not great, but it's deliberate to keep these drivers separate.

Furthermore, Flang should follow https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly.

That file follows the MLIR coding style (as documented in the file):

• llvm-project/flang/lib/Frontend/CompilerInvocation.cpp

  Lines 7 to 11 in 60baaf1

  	//===----------------------------------------------------------------------===//
  	//
  	// Coding style: https://mlir.llvm.org/getting_started/DeveloperGuide/
  	//
  	//===----------------------------------------------------------------------===//

In principle, 2 coding styles are used within Flang:

• MLIR for lowering/codegen/driver
• C++ for Parsing/sema

Now, this is a separate issue and I suggest creating a dedicated ticket for this:

• mostly to avoid discussing multiple issues in one ticket
• this is a matter for all of Flang rather than just the driver

Apparently, the code style divergence is intentional (https://reviews.llvm.org/D125007).

Yes, please follow the MLIR coding style. As documented in the file. If no such header exists, I can fix that (PRs are welcome too).

[jeffhammond]

Do you have more examples?

[pP]arseOptimizationRemark at least.