[cfe-commits] [PATCH] Correct first-line indentation in preprocessor output

[Jordan Rose]

Hm. If this is the change we go with, there should definitiely be a comment saying that MoveToLine(SourceLocation) returns a different value than MoveToLine(unsigned). That, or they should be changed to be consistent.

(My first inclination was to change PrintPPOutputPPCallbacks::HandleFirstTokOnLine, and keep the current semantics of MoveToLine, but then you'd have to call getPresumedLoc again. Or not use the convenience wrapper for MoveToLine. But it's probably okay since, as Eli pointed out, this is the only consumer of the return value.)

Also, you should probably add a ^ to your {{       }} in your test case, just to make things stricter/easier for FileCheck.

Jordan


On Jan 9, 2013, at 15:00 , Hal Finkel <hfinkel at anl.gov> wrote:

> ----- Original Message -----
>> From: "Eli Friedman" <eli.friedman at gmail.com>
>> To: "Hal Finkel" <hfinkel at anl.gov>
>> Cc: "llvm cfe" <cfe-commits at cs.uiuc.edu>
>> Sent: Wednesday, January 9, 2013 4:40:18 PM
>> Subject: Re: [cfe-commits] [PATCH] Correct first-line indentation in preprocessor output
>> 
>> On Wed, Jan 9, 2013 at 2:20 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>>> Currently, the -E output from clang does not produce the correct
>>> indentation on the first line. This is because MoveToLine returns
>>> false, and when this happens, the regular process for producing
>>> initial indentation is skipped. This patch makes sure this does
>>> not happen on the first line -- it is not clear to me whether
>>> there are other circumstances where the current logic could be
>>> problematic.
>>> 
>>> It looks like calling SourceManager::getPresumedLoc is a relatively
>>> expensive operation, so however this is fixed, I assume that we
>>> want to minimize calls to that function.
>>> 
>>> Please review.
>> 
>> --- a/lib/Frontend/PrintPreprocessedOutput.cpp
>> +++ b/lib/Frontend/PrintPreprocessedOutput.cpp
>> @@ -139,10 +139,13 @@ public:
>>                                 diag::Mapping Map, StringRef Str);
>> 
>>   bool HandleFirstTokOnLine(Token &Tok);
>> -  bool MoveToLine(SourceLocation Loc) {
>> +  bool MoveToLine(SourceLocation Loc, bool *FirstLine = 0) {
>>     PresumedLoc PLoc = SM.getPresumedLoc(Loc);
>> -    if (PLoc.isInvalid())
>> +    if (PLoc.isInvalid()) {
>> +      if (FirstLine) *FirstLine = false;
>>       return false;
>> +    }
>> +    if (FirstLine) *FirstLine = PLoc.getLine() == 1;
>>     return MoveToLine(PLoc.getLine());
>>   }
>>   bool MoveToLine(unsigned LineNo);
>> 
>> There is precisely one user of the return value of MoveToLine: the
>> one
>> you're modifying.  Why do we need an extra out parameter?
> 
> Good point, thanks! Revised patch attached -- which now changes one source line ;)
> 
> -Hal
> 
>> 
>> -Eli
>> 
> 
> -- 
> Hal Finkel
> Postdoctoral Appointee
> Leadership Computing Facility
> Argonne National Laboratory
> <first-line-indent-v2.patch>_______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits