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

[Manuel Klimek]

On Tue, Dec 18, 2012 at 12:24 AM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On Dec 17, 2012, at 2:52 PM, Manuel Klimek <klimek at google.com> wrote:
>
> 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.
>>
>> 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).
>>
>
> As far as I'm aware the RewriterTestContext has nothing to do with
> formatting - it has all to do with combining all the dependencies of the
> *Rewriter* into a single instantiable entity. To trigger formatting, you
> need a Lexer, a SourceManager and a configuration - that's all.
>
> So, to summarize what I said in an earlier mail, I would propose the
> following (if we decide we really want something along those lines):
> - come up with an interface that clients like libclang would need; from
> what I can see that would be putting together all the dependencies of the
> Rewriter into an easy-to-instantiate bundle and having a covenience method
> to create an in-memory file on the source manager (from what I saw the
> proposed change does not use all the temp file magic, which is both the
> most brittle and what I don't expect anything bu the test to use)
> - put that into a decently named class in Rewrite/; perhaps
> RewriterDependencies, or RewriterContext, or VirtualRewriterContext (to
> underline the use for virtual files)
>
>
> I think we're mostly in agreement here. VirtualRewriterContext in Rewrite
> would be fine by me.
>
> But to take a step back (and maybe I missed something) - doesn't libclang
> already have a way to instantiate everything needed for Rewriting a bunch
> of files?
>
>
> We're not rewriting files, we're rewriting text produced by calling
> Decl::print(). Since we'll be doing this a lot within libclang, we need to
> keep the context object around so we're not rebuilding the SourceManager,
> Lexer, etc.
>

Makes sense. Sounds like we have a plan then...

Fariborz, anything unclear?

Cheers,
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121218/9f649673/attachment.html>