[flang-commits] [PATCH] D106727: [flang] Produce proper "preprocessor output" for -E option

[Peter Klausler via Phabricator via flang-commits]

klausler marked 4 inline comments as done.
klausler added inline comments.


================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:255-276
+public:
+  // -P: Suppress #line directives in -E output
+  bool noLineDirectives() const { return noLineDirectives_; }
+  FrontendOptions &set_noLineDirectives(bool yes = true) {
+    noLineDirectives_ = yes;
+    return *this;
+  }
----------------
awarzynski wrote:
> klausler wrote:
> > awarzynski wrote:
> > > Do you think that we need accessors and mutators here? We've kept `FrontendOptions` rather basic - it's just a plain aggregate. There's no intention to make it any more "clever". Keeping everything "public" allows us to have a rather basic and concise interface. 
> > > 
> > > If we do add member methods for `noLineDirectives_` and `noLineDirectives_`, then perhaps we should do it for all member variables? But then we'll bloat this class.
> > > 
> > > Also, we've been using bitfields rather than `bool`s for this class to keep it slim. Could we stick to bitfields for consistency?
> > I put them there because you're using names in that class with terminal underscores, and that's a convention in f18 (and google3) meant only for private data members; I didn't want to perpetuate that error.  If everything here is meant to be public, this should be a struct whose members are all data and all public.
> > 
> > Bitfields can't have default initializers, which is the best way to initialize these things; otherwise the default value is separated from the declaration and possibly in another file.
> > I put them there because you're using names in that class with terminal underscores, and that's a convention in f18 (and google3) meant only for private data members; I didn't want to perpetuate that error. If everything here is meant to be public, this should be a struct whose members are all data and all public.
> 
> Ack. Fixed in https://reviews.llvm.org/D107062
> 
> > Bitfields can't have default initializers, which is the best way to initialize these things; otherwise the default value is separated from the declaration and possibly in another file.
> 
> Sounds like each approach has its pros and cons. I would still prefer consistency and to rely on the constructor to initialise everything (which is nearby, in the same file). If you are strongly in favour `bool`s, then let's update all fields at the same time. Preferably in a dedicated patch. 
They've been moved to PreprocessorOptions.


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:378-379
+  } else if (args.hasArg(clang::driver::options::OPT_P)) {
+    // If both -P and -fdebug-dump-cooked-chars appear, -P is
+    // unclaimed and will elicit a warning
+    opts.set_noLineDirectives(true);
----------------
awarzynski wrote:
> I would replace this comment with a diagnostic, e.g. :
> ```lang=c++
> const unsigned diagID = diags.getCustomDiagID(clang::DiagnosticsEngine::Warning, "'-P' will be ignored ('-P' and '-fno-reformat' are incompatible)");
> diags.Report(diagID);
> }
> ```
> This way, the driver becomes a bit more friendly towards end-users. You will have to update this method a tiny bit. Here's a patch with a test (it's a small change):
> 
> ```lang=bash
> diff --git a/flang/lib/Frontend/CompilerInvocation.cpp b/flang/lib/Frontend/CompilerInvocation.cpp
> index 04905f103daf..3d38d6dba3ff 100644
> --- a/flang/lib/Frontend/CompilerInvocation.cpp
> +++ b/flang/lib/Frontend/CompilerInvocation.cpp
> @@ -342,8 +342,10 @@ static std::string getIntrinsicDir() {
>  ///
>  /// \param [in] opts The preprocessor options instance
>  /// \param [out] args The list of input arguments
> -static void parsePreprocessorArgs(
> -    Fortran::frontend::PreprocessorOptions &opts, llvm::opt::ArgList &args) {
> +static bool parsePreprocessorArgs(Fortran::frontend::PreprocessorOptions &opts,
> +    llvm::opt::ArgList &args, clang::DiagnosticsEngine &diags) {
> +  unsigned numErrorsBefore = diags.getNumErrors();
> +
>    // Add macros from the command line.
>    for (const auto *currentArg : args.filtered(
>             clang::driver::options::OPT_D, clang::driver::options::OPT_U)) {
> @@ -372,13 +374,24 @@ static void parsePreprocessorArgs(
>          ? PPMacrosFlag::Include
>          : PPMacrosFlag::Exclude;
> 
> +
>    if (args.hasArg(clang::driver::options::OPT_fdebug_dump_cooked_chars)) {
>      opts.set_dumpCookedChars(true);
> -  } else if (args.hasArg(clang::driver::options::OPT_P)) {
> -    // If both -P and -fdebug-dump-cooked-chars appear, -P is
> -    // unclaimed and will elicit a warning
> +  }
> +
> +  if (args.hasArg(clang::driver::options::OPT_P)) {
>      opts.set_noLineDirectives(true);
>    }
> +
> +  if (opts.noLineDirectives() && opts.dumpCookedChars()) {
> +    const unsigned diagID =
> +        diags.getCustomDiagID(clang::DiagnosticsEngine::Warning,
> +            "'-P' will be ignored ('-P' and '-fno-reformat' are incompatible)");
> +    diags.Report(diagID);
> +    opts.set_noLineDirectives(false);
> +  }
> +
> +  return diags.getNumErrors() == numErrorsBefore;
>  }
> 
>  /// Parses all semantic related arguments and populates the variables
> @@ -541,7 +554,7 @@ bool CompilerInvocation::CreateFromArgs(CompilerInvocation &res,
>    }
> 
>    success &= ParseFrontendArgs(res.frontendOpts(), args, diags);
> -  parsePreprocessorArgs(res.preprocessorOpts(), args);
> +  success &= parsePreprocessorArgs(res.preprocessorOpts(), args, diags);
>    success &= parseSemaArgs(res, args, diags);
>    success &= parseDialectArgs(res, args, diags);
>    success &= parseDiagArgs(res, args, diags);
> diff --git a/flang/test/Driver/pp-flags-warning.f90 b/flang/test/Driver/pp-flags-warning.f90
> new file mode 100644
> index 000000000000..5fbf3a1574a5
> --- /dev/null
> +++ b/flang/test/Driver/pp-flags-warning.f90
> @@ -0,0 +1,8 @@
> +! REQUIRES: new-flang-driver
> +
> +! RUN: %flang_fc1 -E -P -fdebug-dump-cooked-chars %s 2>&1 | FileCheck %s
> +
> +! CHECK: warning: '-P' will be ignored ('-P' and '-fno-reformat' are incompatible)
> +
> +program hello
> +end
> ```
> Feel free to use this (with or without edits).
Now that -fno-reformat is a developer-only flag, this is less important.  Changing to accept both.  The output of -fno-reformat has no `#line` directives so the -P is at worst redundant when both appear.


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

https://reviews.llvm.org/D106727