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

[Peter Klausler via Phabricator via flang-commits]

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


================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:31-33
+  /// -fdebug-dump-cooked-chars
+  DebugDumpCookedChars,
+
----------------
awarzynski wrote:
> Not needed, `-fdebug-dump-cooked-chars` is not an action.
It used to be so in my first patch.  Removed.


================
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:
> 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.


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

https://reviews.llvm.org/D106727