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

Tue Jul 20 10:55:16 PDT 2021

aaron.ballman added a comment.

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

> In D104601#2877855 <https://reviews.llvm.org/D104601#2877855>, @aaron.ballman wrote:
>
>> 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.
>
> I made the flag an error if used without `-E`, so the build system cannot add it.
>
> Flags such as `-P`, `-fuse-line-directives`, `-frewrite-imports` and `-frewrite-includes` are also silently ignored without `-E`. This requirement seems exclusive for `-felide-unnecessary-whitespace`. It also suggests that it is not a problematic scenario.

Perhaps it's not a problematic scenario, but I'd consider those cases to also be bugs. Silently ignoring things the user wrote (either in code or on the command line) is typically user-hostile.

>>>> 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.
>
> I ran it (using ccache) over llvm-project and the llvm-test-suite. Is there another large corpus do you have in mind?

A Linux or BSD distro worth of code would be really nice, but I don't insist. The trouble with llvm-project is that it has a somewhat uniform style for code, and I've found that tools in distro packages tend to exercise more interesting cases sometimes.

>> I also think we should rename it because "normalize whitespace" can be ambiguous depending on what context you're reading the words in.
>
> Changed to your suggested `-felide-unnecessary-whitespace`, although I think name will not allow use to insert whitespace e.g. for keeping the line length down. If that's not required, I would prefer `-fminimize-whitespace` over -felide-unnecessary-whitespace`.

I'd be perfectly happy with `-fminimize-whitespace` as well if that's your preference.

================
Comment at: clang/docs/ClangCommandLineReference.rst:2484
+-P option to normlize whitespace such that two files with only formatted
+changes are equal.
+
----------------
Meinersbur wrote:
> aaron.ballman wrote:
> > Should this option be ignored when not performing a preprocessing action (and documented as such)?
> I changed it to an error if used without `-E`.
Thanks! Can you add that info to the documentation here?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104601