[cfe-commits] [PATCH] Initial version of formatting library

[Daniel Jasper]

On Sat, Dec 1, 2012 at 4:20 PM, Daniel Jasper <djasper at google.com> wrote:
> On Fri, Nov 30, 2012 at 11:50 AM, Douglas Gregor <dgregor at apple.com> wrote:
>>
>> On Nov 30, 2012, at 11:32 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>> On Fri, Nov 30, 2012 at 8:18 PM, Douglas Gregor <dgregor at apple.com> wrote:
>>>
>>>
>>> On Nov 29, 2012, at 1:16 PM, Daniel Jasper <djasper at google.com> wrote:
>>>
>>> >
>>> >  Ping.
>>> >
>>> >  Also, we are continuing the development on
>>> > https://github.com/djasper/clang/tree/format, but I am currently not
>>> > updating this patch in fear of prolonging the review even more. It has
>>> > reached a stage where it starts improving my workflow thanks to a little vim
>>> > integration I have hacked together ...
>>>
>>> A few questions and comments on the patch, but IMO I'd rather see this go
>>> into the main Clang tree soon and get hacked on there, since it looks like
>>> we're moving in the right direction and it's obviously functionality we want
>>> in the core.
>
> Glad to hear, that is what I was hoping for :-).
>
>> +/// \brief Reformats the given Ranges in the token stream coming out of \c Lex.
>> +///
>> +/// Ranges are extended to include full unwrapped lines.
>> +/// TODO(alexfh): Document what unwrapped lines are.
>> +tooling::Replacements reformat(const FormatStyle &Style, Lexer &Lex,
>> +                               SourceManager &SourceMgr,
>> +                               std::vector<CodeRange> Ranges);
>>
>> As the main entry point, it seems that this really needs a more extensive comment. In general, please provide comments for the various new classes (FormatStyle, Formatter, UnwrappedLineFormatter, etc.), so people can navigate the code.
>
> Yes. I will fix / document it. Basic question about the approach, just
> to be sure we are on the same page: Since you are commenting on the
> current version of the github repository, should I polish that a bit
> and send an updated version out for review?
>
>>
>> +  bool isIfForOrWhile(Token Tok) {
>> +    if (Tok.getKind() != tok::raw_identifier)
>> +      return false;
>> +    StringRef Data(SourceMgr.getCharacterData(Tok.getLocation()),
>> +                   Tok.getLength());
>> +    return Data == "for" || Data == "while" || Data == "if";
>> +  }
>>
>> This is one of many places where we're doing string comparisons against keywords, which seems needlessly inefficient. How about using an IdentifierTable (with keywords filled in) to resolve identifiers to keywords, so you can actually check the tokens? That'll also be smarter w.r.t. identifiers that are keywords in some dialects but not others.
>
> Basically, we want to reuse Preprocessor::getIdentifierInfo(), right?
> I will look into that.

Sorry, of course IdentifierTable is exactly what we want to use.. /me
needs to read email more closely...

>>> +/// \brief A character range of source code.
>>> +struct CodeRange {
>>> +  CodeRange(unsigned Offset, unsigned Length)
>>> +    : Offset(Offset), Length(Length) {}
>>> +
>>> +  unsigned Offset;
>>> +  unsigned Length;
>>> +};
>>>
>>> Why isn't this a SourceRange or CharSourceRange?
>>
>>
>> This would put the burden on the client to do the "Offset+Length ->
>> SourceRange" translation. But you're right, in this case that might be the
>> right trade-off…
>>
>>
>> This seems like a *very* small burden to eliminate two
>> similar-but-not-the-same concepts with disturbingly similar names. It's
>> CharSourceRange you want, though.
>
> Ok, I will remove CodeRange here. The reasoning behind it was that
> both SourceRange and CharSourceRange effectively work with
> SourceLocations and SourceLocations provide locations based on a
> SourceManager. We thought that the library might have users that do
> not actually have a SourceManager...
>
>> Once the first version is checked in, we'll next check a tool into
>> clang-extra-tools plus some nice vi integration with which you can run it
>> over code snippets (or full files). Daniel actually has built that, and the
>> fact that this is already surprisingly useful has accelerated development
>> here somewhat :)
>>
>>
>> Okay!
>>
>> - Doug
>
> Thanks for taking a look!
> Daniel