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

Douglas Gregor dgregor at apple.com
Mon Dec 17 15:24:59 PST 2012


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.

	- Doug

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121217/6f1e68aa/attachment.html>


More information about the cfe-commits mailing list