[PATCH] D37014: [clang-tidy] Add a checker to remove useless intermediate variables before return statements with comparisons

[Tristan Bourvon via Phabricator via cfe-commits]

tbourvon marked 4 inline comments as done.
tbourvon added a comment.

(By the way, credits go to @Abpostelnicu for the latest changes regarding MaximumLineLength interop with clang-format options)



================
Comment at: clang-tidy/readability/UnnecessaryIntermediateVarCheck.cpp:376
+    // expression wouldn't really benefit readability. Therefore we abort.
+    if (NewReturnLength > MaximumLineLength) {
+      return;
----------------
Abpostelnicu wrote:
> lebedev.ri wrote:
> > Is there really no way to format the fixes, and *then* check the line length?
> > ```
> > $ clang-tidy --help
> > ...
> >   -format-style=<string>       - 
> >                                  Style for formatting code around applied fixes:
> >                                    - 'none' (default) turns off formatting
> >                                    - 'file' (literally 'file', not a placeholder)
> >                                      uses .clang-format file in the closest parent
> >                                      directory
> >                                    - '{ <json> }' specifies options inline, e.g.
> >                                      -format-style='{BasedOnStyle: llvm, IndentWidth: 8}'
> >                                    - 'llvm', 'google', 'webkit', 'mozilla'
> >                                  See clang-format documentation for the up-to-date
> >                                  information about formatting styles and options.
> >                                  This option overrides the 'FormatStyle` option in
> >                                  .clang-tidy file, if any.
> > ...
> > ```
> > so `clang-tidy` is at least aware of `clang-format`.
> I think this is doable since I see this in the code:
> 
> https://code.woboq.org/llvm/clang-tools-extra/clang-tidy/ClangTidy.cpp.html#199
> 
> That leads me to think that we can have this before applying the fixes and in case the fix after re-format has a line that violates our rule it gets dropped. I'm gonna update the patch with this new addon.
Regarding this comment, Andi (@Abpostelnicu) and I have analyzed the situation and we believe that formatting the replacement before checking the line length achieves almost nothing, yet complicates the code a lot.

If we format the replacement before applying the fix, then we're //almost// sure that clang-format will split the replacement into multiple lines and that it will never go above the maximum line length (except for extremely rare cases like 80+ characters identifiers).
Actually, we believe that splitting the return statement into multiple lines hinders its readability, and therefore that in cases where the fix would exceed the maximum line length, we're better off not applying it, since the main goal of this check **is** readability.

clang-format can still be run after the fix is applying, of course.


https://reviews.llvm.org/D37014