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

Matt Beaumont-Gay matthewbg at google.com
Thu Mar 22 16:40:49 PDT 2012

A few general comments:
- Doxygen comments throughout, please.
- Lines should be shorter than 80 columns.
- No extra whitespace inside delimiters -- "if (foo)", not "if ( foo
)", and similarly for [] and <>.
- Use StringRef in preference to passing a char* and length (e.g. in
- Get to know the naming rules in the style guide, and adjust
capitalization accordingly:
- Also per the naming rules, the RewriteIncludes class should be
renamed as a noun, e.g. IncludeRewriter.


+++ RewriteIncludes.cpp

  std::list< FileChange > FileChanges;

Prefer std::vector -- no reason to use std::list here.

  if (UseLineDirective) {
    OS << "#line" << ' ' << line << ' ' << '"' << filename;
    OS << '"';

No need to break this sequence of output strings across two lines.

    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?

    if (!FileChanges.empty()) { // there may be internal sort-of includes

What "sort-of includes"?

  for (std::list< FileChange >::const_iterator it
       = FileChanges.begin(), end = FileChanges.end();

Funny line break here -- prefer breaking after the comma.

      if (LastChar != '\n' && LastChar != '\r')
        OS << '\n';

As much as I hate files with \r line endings, I also would like us to
not write files with *mixed* line endings...

    + 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.

// 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. Also, indent
the comment to the same level as the surrounding code.

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

Please elaborate?

            StringRef I = NextIdentifierName(RawLex, RawToken);

Use a more descriptive variable name. 'I' is particularly bad because
it usually denotes a loop counter or iterator.

  // 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?

--- a/include/clang/Lex/Preprocessor.h
+++ b/include/clang/Lex/Preprocessor.h
@@ -626,6 +633,11 @@ public:
     while (Result.getKind() == tok::comment);

+  /// Disables macro expansion everywhere except for preprocessor directives.
+  void SetMacroExpansionOnlyInDirectives() {
+    DisableMacroExpansion = true;
+    MacroExpansionInDirectivesOverride = true;
+  }
   /// LookAhead - This peeks ahead N tokens and returns that token without

Add a blank line after the method body.

+++ b/test/Frontend/rewrite-includes.c
@@ -0,0 +1,77 @@
+// RUN: %clang_cc1 -verify -rewrite-includes -DFIRST %s -o %t

// RUN: %clang_cc1 -rewrite-includes -DFIRST %s -o - | FileCheck
-strict-whitespace %s

Also, the various *.h files should go in the Inputs subdirectory (and
add -I %S/Inputs to the RUN line).

On Mon, Mar 19, 2012 at 13:43, Lubos Lunak <l.lunak at suse.cz> wrote:
> On Friday 16 of March 2012, Lubos Lunak wrote:
>> On Friday 16 of March 2012, Chandler Carruth wrote:
>> > I've not looked at the patch in detail, but a simple idea: #if 0 block,
>> > with a comment explaining why.
>>  That's a very good idea and it seems to work nicely.
>>  I've also added commenting out #pragma once, and now the patch should be
>> hopefully fully ready. I've also done a LibreOffice build again and this
>> time there wasn't a single problem.
>  I've still found two minor details, and also made it noticeably faster while
> I was at it. But now it's perfect, honest :).
>  The latest version is attached. This time it is a patch and separately the
> new source file (to be put in clang/lib/Rewrite), to make review it easier. I
> also do not intend to make further changes until reviewed.
>  The changes needed elsewhere are:
> - adding the new option and invoking the functionality, trivial duplication
> of -rewrite-macros
> - changes in Lexer class to not access the (null) Preprocessor pointer in raw
> mode, which the code normally uses, but temporarily switches to
> ParsingPreprocessorDirective mode (apparently a use case that's not been
> needed yet)
> - addition of MacroExpansionInDirectivesOverride flag to Preprocessor, which
> makes it possible to avoid macro expansion everywhere else except in macro
> directives, which reduces the time to process files to less than a half (and
> to less than with plain -E, making icecream's distributed compilation
> actually faster this way). It could be made even faster if the lexer could be
> switched to mode where it would parse only comments and preprocessor
> directives (nothing else matters in this case), and I expect it would be an
> additional boost to distributed compiles, as this step is the bottleneck, but
> maybe later ;).
> - and a test, which of course passes. Moreover, as already said, I've
> successfully compiled LibreOffice using this feature.
> --
>  Lubos Lunak
>  l.lunak at suse.cz
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits

More information about the cfe-commits mailing list