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

[Andrzej Warzynski via Phabricator via flang-commits]

awarzynski added a comment.

In D106727#2910910 <https://reviews.llvm.org/D106727#2910910>, @klausler wrote:

>> As for the spelling itself, -fdebug-dump-cooked-chars will not dump anything on its own. It needs to be paired with -E. So the name is a bit misleading. Perhaps we could have something like -fcooked-only or -fno-reformat? Naming is hard, I have no particular preferences.
>
> Tell me what you want, and I'll change the patch to use it.  Otherwise I'll have to iterate.

`-fno-reformat`, please.



================
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;
+  }
----------------
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. 


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

https://reviews.llvm.org/D106727