On Sat, Dec 1, 2012 at 4:20 PM, Daniel Jasper <[email protected]> wrote:
> On Fri, Nov 30, 2012 at 11:50 AM, Douglas Gregor <[email protected]> wrote:
>>
>> On Nov 30, 2012, at 11:32 AM, Manuel Klimek <[email protected]> wrote:
>>
>> On Fri, Nov 30, 2012 at 8:18 PM, Douglas Gregor <[email protected]> wrote:
>>>
>>>
>>> On Nov 29, 2012, at 1:16 PM, Daniel Jasper <[email protected]> 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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
