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

[David Blaikie]

I've updated this patch based on the feedback as follows:

On Tue, Apr 3, 2012 at 5:32 PM, Matt Beaumont-Gay <matthewbg at google.com> wrote:
> Looking better.
>
> On Sat, Mar 31, 2012 at 14:32, Lubos Lunak <l.lunak at suse.cz> wrote:
>>> >  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.
>>>
>>> Can you use the FileSkipped() callback to make this any more efficient?
>>
>> No. If you are asking about the quoted part above, then it's not about
>> efficiency, it's about correct/incorrect. If it was a question in general,
>> then it's more efficient (and probably also correct, given the action is
>> supposed to rewrite includes) to just comment out everything instead of
>> figuring out exactly what needs it or not.
>
> So, any #include directive should result in either a FileChanged() or
> FileSkipped() callback. Build up a vector with a record of what
> happened for each #include. Then, when you re-lex in Process(), just
> keep an iterator into the vector, and increment it every time you see
> another #include.

I went a slightly different route, though I'm not wedded to it & could
be persuaded in favor of this.

I took advantage of the FileSkipped callback to avoid what I think was
some undefined behavior (using the 'Id' and 'FileType' members of a
FileChange that had never been initialized because the FileChanged
callback never occured - this seemed to get lucky enough & bail out in
the validity check at the start of RewriteIncludes::Process, which
I've since replaced with an assert) - by removing the 'pending' entry
from the map whenever a FileSkipped callback is received.

>>> A few new comments, in addition to the general style issues which are
>>> still unaddressed (doxygen, whitespace, etc):
>>
>>  I have already converted function/type comments into doxygen comments, and
>> there's no point in converting code comments. If you meant to say something
>> else with "Doxygen comments throughout", you didn't actually say it.
>
> Functions and methods should have doxyments explaining the purpose of
> the function and the meaning of each parameter. Class data members
> should also have (brief) doxyments.

Doxycomments added.

> A few more things, mostly nits:
>
> Throughout, be consistent about whether the * or & is next to the type
> or the variable. Prevalent style in Clang is to put it next to the
> variable.

Consistified * and & as per LLVM coding style.

> Comments should begin with a capital letter (and if they're complete
> sentences, end with a period.)
>
>  bool Process(FileID FileId, SrcMgr::CharacteristicKind Type);
>
> 'Type' is already pretty overloaded in Clang; probably should call
> this 'FileType.'

Renamed 'Type' to 'FileType'.

> ::IncludeRewriter::IncludeRewriter(Preprocessor &pp, raw_ostream &os,
>  bool lineMarkers)
>  : PP(pp), SM(PP.getSourceManager()), OS(os),
>  DisableLineMarkers(lineMarkers), LastInsertedFileChange(0) {
>
> The constructor parameter should probably be called
> 'DisableLineMarkers' so it's obvious that passing 'true' does not mean
> "yes, I want line markers."

Corrected (& renamed to ShowLineMarkers (& actually implemented the
semantics - the flag was passed but unused) so as not to needlessly
invert the name/logic compared to the existing
PreprocessorOutputOptions) and added a test case for this flag.

> Also, the continuation of the parameter list should be indented to the
> opening paren, similarly to most of the other method definitions;
> likewise for argument lists on function calls throughout. And, no need
> for the leading '::' here.
>
>      OS << " 3";
>
> These magic strings should have comments; bonus points for references
> to authoritative documentation.

Commented with quotes and references to authoritative documentation.

>
>  if (Reason == EnterFile) {
>
> Invert the condition and return early:
> http://llvm.org/docs/CodingStandards.html#hl_earlyexit

Inverted the condition and returned early.

>
>        FileChange& Ref = FileChanges[LastInsertedFileChange];
>
> Name this variable 'Change' or such.

Variable removed in favor of alternative implementation.

>
>  FileChangeMap ::const_iterator Find = FileChanges.find(Loc.getRawEncoding());
>
> 'I' (or 'Result' if you're feeling verbose).

Variable renamed to 'I'.

>
>  if(Find != FileChanges.end())
>
> Space between 'if' and '(':
> http://llvm.org/docs/CodingStandards.html#micro_spaceparen

Spaces added before condition '(' across all uses.

> /// Copies next yet written file content up (and not including writeEnd).
>
> This could probably be worded a little more clearly. Maybe "Writes
> bytes from FromFile, starting at \p NextToWrite and ending at \p
> WriteEnd - 1."

Rephrased as suggested (& some renaming too).

> void IncludeRewriter::OutputContentUpTo(unsigned WriteEnd,
>                                        unsigned& NextToWrite,
>                                        int& Lines, const char* EOL,
>                                        const MemoryBuffer* FromFile,
>                                        bool EnsureNewline) {
>
> Input parameters should come before output parameters.

Not entirely possible (EnsureNewLine is an input parameter but has a
default argument) nor necessarily desirable (the begin/end range
described by "WriteFrom" and "WriteTo" makes sense together) & I don't
think we really have any policy (or consistency) on this in the
codebase - though I don't mind the idea as a very light guidance when
all else is equal.

>  int Lines = 1; // current input file line number
>
> I'd call this just 'Line'.

Renamed to 'Line'.

>            // (clang sometimes optimizes and does not repeatedly include some
>            // files even though it should, so all includes need to be
>            // commented, otherwise valid directives would be left in)
>
> To be more precise: "Clang skips repeated includes of headers which
> have a multiple-inclusion guard macro, so..."

Dropped the comment here, though the behavior is described elsewhere
(see the doxycomment for InclusionRewriter::FileSkipped, for example),
as this seems fairly self explanatory now. Added a little bit of this
comment down

>      RawLex.setParsingPreprocessorDirective(false);
>      RawLex.SetCommentRetentionState(false);
>
> Why do we need to re-disable the comment retention state here?

No reason that I could divine. Removed.

> +void RewriteIncludesInInput(Preprocessor &PP, raw_ostream *OS, const
> PreprocessorOutputOptions &Opts);
>
> 80 columns

Fixed.

> +class Preprocessor::ResetMacroExpansionHelper
> +{
>
> Brace should be on the same line as the class name

Fixed.

> +  RewriteIncludesInInput(CI.getPreprocessor(), OS,
> CI.getPreprocessorOutputOpts());
>
> 80 columns

Fixed.

> +// RUN: %clang_cc1 -verify -rewrite-includes -DFIRST -I %S/Inputs %s -o %t
> +// RUN: cat %t | FileCheck -strict-whitespace %s
>
> No need to involve a tempfile here, just pass '-' for the clang output
> file and pipe directly to FileCheck. (And don't worry about making it
> fit in 80 columns.)

Done.

> +// CHECK: {{^}}// STARTCOMPARE{{$}}
> +// CHECK-NEXT: {{^}}#define A(a,b) a ## b{{$}}
>
> It might be clearer and more maintainable to interleave these
> CHECK-NEXTs with the actual input above. I don't feel strongly about
> this, though.

Interleaving will get a bit confusing given the current way this test
is written to use CHECK-NEXTs - since the comments containing the
checks will show up in the preprocessed output & thus get in the way
of the checks.
I'm wondering whether it'd just be clearer to do an exact match
against a separate file, really...

> Here's another test case (to be #include'd twice):
> #ifndef REWRITE_INCLUDES_7
> #define REWRITE_INCLUDES_7
> included_line7
> #endif

Added.

& I also renamed the IncludeRewriter to InclusionRewriter (since the
former could still be confused as a verb phrase) and renamed the
source file it was in to match (lib/Rewrite/InclusionRewriter.cpp)

Thanks,
- David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rewrite-includes.patch
Type: application/octet-stream
Size: 36120 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120517/6c3cec42/attachment.obj>