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

[Manuel Klimek]

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)

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?

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