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

[Matt Beaumont-Gay]

Looks good, just a few style nits:

+/// Count the raw \n characters in the \p Len characters from \p Pos.

Does Doxygen do the right thing with "\n"?

+  if (Pos+1 < FromFile.getBufferEnd() && Pos[1] == '\r')

Whitespace around binary operators (and below).

+  Line += CountNewLines(FromFile.getBufferStart() + WriteFrom,
+    WriteTo - WriteFrom);

Prevailing style is to indent the line continuation to the opening
paren of the function call.

+  const char *FileName = FromFile.getBufferIdentifier();

Trailing whitespace.

+  // `1' indicates the start of a new file

The backticks in these comments keep throwing me off... maybe here
(which is a ways below the link to the GNU preprocessor docs), add
"Per the GNU docs:" or such.

+            // keep the directive in, commented out

Per the style guide: "When writing comments, write them as English
prose, which means they should use proper capitalization, punctuation,
etc."

On Thu, May 17, 2012 at 1:12 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 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