[cfe-commits] clang formatter patch to move and rename RewriterTestContext.h

[Daniel Jasper]

On Mon, Dec 17, 2012 at 11:04 PM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On Dec 17, 2012, at 9:55 AM, Daniel Jasper <djasper at google.com> wrote:
>
> Hi Fariborz,
>
> I am taking a look at your patch, but please bear with me for another day
> or two. I am not yet convinced that mingling C++ and ObjC formatting that
> closely is a wise decision. I think we might need something that clearly
> separates the different modes. And you touch a lot of code that I intended
> to refactor into something halfway sane this week. I will take your patch
> into consideration when doing so, but it might need some changes afterwards.
>
>
> If we're going to distinguish the modes, we need to do so syntactically.
> Objective-C++ is a very popular dialect; we can't simply rely on extrinsic
> knowledge of whether we're dealing with Objective-C or C++ to make these
> decisions.
>

I am not sure I fully understand this. We are currently relying on a Lexer
which in turn requires LangOptions to create the correct tokens. Same for
the identifier table we use to split raw_identifiers into identifiers and
keywords. Are you saying the formatter should not capitalize on this
knowledge of the input token stream?


>
> As for RewriterTestContext, please try to figure out whether that is
> really something you want to reuse in libclang. If so, I am happy to move
> it into include/clang/Tooling (or you could do that yourself). But I
> suspect that we want something different there, possibly a completely
> different interface.
>
>
> It is. At least, we need a persistent context that we can use to apply the
> code formatter to code snippets produced within libclang. We could pull out
> just the parts of RewriterTestContext that we need and give it a new name
> (SimpleFormatContext, for example).
>
> - Doug
>
> On Mon, Dec 17, 2012 at 6:47 PM, jahanian <fjahanian at apple.com> wrote:
>
>>
>> On Dec 17, 2012, at 9:36 AM, Manuel Klimek <klimek at google.com> wrote:
>>
>> I don't think it's the right design to put something that is generally
>> thought for testing into a public header and use it from libclang.
>> If we want something similar to RewriterTestContext, it should:
>> a) live next to the Rewriter in clang - as it provides everything needed
>> to rewrite files
>> b) leave out all functionality that's not needed outside of tests
>> c) be proposed and reviewed in an extra patch, which I'd assume would
>> need approval from Doug - it's a non-trivial addition to the public api
>>
>>
>> I agree with all these. I wanted to move forward with ObjC formatting and
>> it was a temporary refactoring allowing me to move forward. Form and
>> placement
>> of the public header rests with Daniel. Also my patch includes complete
>> formatting of ObjC declarations which I hope will be useful.
>>
>> - Fariborz
>>
>>
>> Now, I'm all for having some nicer "just bundle me something up" APIs so
>> a lot of Clang's infrastructure like the Rewriter gets simpler to use, but
>> I'd rather see it introduced in a deliberate way.
>>
>> Thanks!
>> /Manuel
>>
>>
>>
>> On Mon, Dec 17, 2012 at 6:13 PM, jahanian <fjahanian at apple.com> wrote:
>>
>>> Hi Daniel,
>>>
>>> Are you going to provide the new public header in clang/Tooling this
>>> week? I have gone ahead and made the necessary changes for formatting ObjC
>>> declarations and
>>> and provide the tests in the attached patch which enforces our
>>> convention for formatting them.  Here is my complete patch (including the
>>> plumbing for
>>> declaration tags which you may ignore).  How do you want to proceed?
>>>
>>> - Thanks, Fariborz
>>>
>>>
>>>
>>> On Dec 16, 2012, at 10:21 PM, Daniel Jasper <djasper at google.com> wrote:
>>>
>>> I think the RewriterTestContext should remain in Tooling as it has
>>> nothing to do with formatting. I guess, we could make it a public header
>>> there (clang/Tooling). And it should remain being called
>>> RewriterTestContext.
>>>
>>> The ObjC change looks fine as a temporary fix. Especially the part in
>>> Format.cpp will undergo a rewrite anyway, hopefully this week. I will look
>>> into what else needs to change to support ObjC. At the very least, we
>>> probably shouldn't set CPlusPlus LangOpts for the Lexer..
>>>
>>>
>>>
>>> On Wed, Dec 12, 2012 at 8:50 PM, Sean Silva <silvas at purdue.edu> wrote:
>>>
>>>> On Wed, Dec 12, 2012 at 1:00 PM, jahanian <fjahanian at apple.com> wrote:
>>>> > ter. This patch is an attempt to have a header in Format directory
>>>> > that others can use. Any suggestion is welcome.
>>>>
>>>> +djasper
>>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>>
>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121218/575a4939/attachment.html>