[PATCH] D104601: [Preprocessor] Implement -fnormalize-whitespace.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D104601#2848366 <https://reviews.llvm.org/D104601#2848366>, @Meinersbur wrote:

> In D104601#2847400 <https://reviews.llvm.org/D104601#2847400>, @aaron.ballman wrote:
>
>> ... would add that it's very common for implementers to ask developers to run their code through `-E` mode to submit preprocessed output in order to reproduce a customer issue with the compiler, and I worry that uses of this flag will have unintended consequences in that scenario.
>
> Why would one add `-fnormalize-whitespace` for this use case?

I don't think they'd *add* it, I worry it's already used by their build system for reasons unknown and the developer replicates it when reporting the bug because they're not looking at what flags can be removed.

>> The "very long line" example mentioned by @dblaikie is sort of along these lines (sorry for the bad pun).
>
> I can add forced line breaks (after/before 80 cols? configurable?) if requested.

I don't think that's necessary just yet. If there's an issue in practice, it seems like we could address it once we have the real world use in front of us. However, It'd help my confidence if you were able to run this functionality over a large corpus of code to see if any issues do pop out, if that's plausible for you.

That said, I think I've convinced myself that this functionality is reasonable (if a bit novel), but I'd like to see it diagnosed when used outside of a preprocessor invocation because it really serves no purpose there. I also think we should rename it because "normalize whitespace" can be ambiguous depending on what context you're reading the words in.



================
Comment at: clang/docs/ClangCommandLineReference.rst:2484
+-P option to normlize whitespace such that two files with only formatted
+changes are equal.
+
----------------
Should this option be ignored when not performing a preprocessing action (and documented as such)?


================
Comment at: clang/include/clang/Driver/Options.td:1789
   PosFlag<SetTrue, [CC1Option], "Use #line in preprocessed output">, NegFlag<SetFalse>>;
+defm normalize_whitespace : BoolFOption<"normalize-whitespace",
+  PreprocessorOutputOpts<"NormalizeWhitespace">, DefaultFalse,
----------------
I think I'd feel most comfortable if this didn't use "normalize" in the name. When I hear that, I think it's going to be doing something like converting all (perhaps funky Unicode) notions of whitespace into ASCII space characters. But this option doesn't actually do that -- it's removing as much whitespace from the preprocessed output as possible. Would it be better named as `-felide-unnecessary-whitespace` or something along those lines?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104601