[cfe-commits] [PATCH] -rewrite-includes

[Lubos Lunak]

On Friday 23 of March 2012, Matt Beaumont-Gay wrote:
> A few general comments:

> - No extra whitespace inside delimiters -- "if (foo)", not "if ( foo
> )", and similarly for [] and <>.
> - Get to know the naming rules in the style guide, and adjust
> capitalization accordingly:
> http://llvm.org/docs/CodingStandards.html#ll_naming

 The thing is, I do know about that page and I tried to follow it, so 
apparently I can't do it on my own (and the fact that the codebase is 
inconsistent on this anyway and I find this style poorly readable is not 
helping). What is the command to automatically format the file to fit your 
requirements?

>   std::list< FileChange > FileChanges;
>
> Prefer std::vector -- no reason to use std::list here.

 I've meanwhile confirmed that it should be actually std::map for noticeably 
better performance.

> - Use StringRef in preference to passing a char* and length (e.g. in
> WriteLineInfo):
> http://llvm.org/docs/ProgrammersManual.html#string_apis

>     if (type == SrcMgr::C_System)
>       OS.write(" 3", 2);
>     else if (type == SrcMgr::C_ExternCSystem)
>       OS.write(" 3 4", 4);
>
> Why OS.write and not operator<< here?

 Because I reused code from PrintPreprocessedOutput.cpp .

> - Lines should be shorter than 80 columns.

>   for (std::list< FileChange >::const_iterator it
>        = FileChanges.begin(), end = FileChanges.end();
>
> Funny line break here -- prefer breaking after the comma.

 You need to pick your poison. C++ code generally does not fit into 80 columns 
and look nice at the same time.

>   OutputContentsUpTo(SM.getFileOffset(DirectiveToken.getLocation())
>     + DirectiveToken.getLength(), nextToWrite, FromFile);
>
> If you have to break a line on one side of a binary operator, prefer
> to break after the operator rather than before.

 In addition to the above, why should I do that? The operator makes it obvious 
why the previous line continues there.

> // It might be beneficial to have a switch that will remove contents of
> lines // that are irrevelant for compile, i.e. comments. Comments are
> actually // the majority of the resulting file and cleaning up such lines
> would // significantly reduce the size of the resulting file without having
> any // effect on any following usage (with the exception of human
> inspection).
>
> The second sentence here is speculative. Remove the comment entirely,
> or keep the first sentence with "FIXME:" at the front.

 It is not speculative. There's also nothing broken to fix, it is a suggestion 
for an alternative mode that would have its advantages and disadvantages.

>             // clang sometimes optimizes and does not repeatedly include
> some // files even though it should,
>
> Please elaborate?

 Clang does not even bother to perform repeated inclusion of the same file if 
it somehow detects it's unnecessary, but the avoided FileChanged() callback 
would mean -rewrite-includes would otherwise leave some #include directives 
in the result that would be performed the next time.

>   // It would be even faster if the preprocessor could be switched to a
> mode // where it would parse only preprocessor directives and comments,
> nothing // else matters for parsing or processing.
>
> Technically true, I'm sure, but have you profiled it and found that it
> would actually matter?

 I have done some profiling, yes, and lexing is the majority of time. And 
scanning only for / and \n simply has to be faster. But that is a possible 
improvement for the future and I will not work on this now (although the tone 
of your mail sounds like new potential contributors need to have their zeal 
thoroughly tested by requiring them to submit something that is perfect, in 
case they soon break down from all the nitpicks and run away before making 
any further improvements).

 Updated version attached.

-- 
 Lubos Lunak
 l.lunak at suse.cz
-------------- next part --------------
A non-text attachment was scrubbed...
Name: RewriteIncludes.cpp
Type: text/x-c++src
Size: 13508 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120323/b68133bf/attachment.cpp>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rewrite-includes.patch
Type: text/x-diff
Size: 14892 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120323/b68133bf/attachment.patch>