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

[Matt Beaumont-Gay]

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.

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

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.

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

::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."

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.

  if (Reason == EnterFile) {

Invert the condition and return early:
http://llvm.org/docs/CodingStandards.html#hl_earlyexit

        FileChange& Ref = FileChanges[LastInsertedFileChange];

Name this variable 'Change' or such.

  FileChangeMap ::const_iterator Find = FileChanges.find(Loc.getRawEncoding());

'I' (or 'Result' if you're feeling verbose).

  if(Find != FileChanges.end())

Space between 'if' and '(':
http://llvm.org/docs/CodingStandards.html#micro_spaceparen

/// 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."

void IncludeRewriter::OutputContentUpTo(unsigned WriteEnd,
                                        unsigned& NextToWrite,
                                        int& Lines, const char* EOL,
                                        const MemoryBuffer* FromFile,
                                        bool EnsureNewline) {

Input parameters should come before output parameters.

  int Lines = 1; // current input file line number

I'd call this just '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..."

      RawLex.setParsingPreprocessorDirective(false);
      RawLex.SetCommentRetentionState(false);

Why do we need to re-disable the comment retention state here?

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

80 columns

+class Preprocessor::ResetMacroExpansionHelper
+{

Brace should be on the same line as the class name

+  RewriteIncludesInInput(CI.getPreprocessor(), OS,
CI.getPreprocessorOutputOpts());

80 columns

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

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

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