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