Two patches

Hi @clang,

I've attached two patches and hope that both will be soon or later committed. I've subdivided them, because the first one is very tiny and was already discussed here.
In particular:

parentmap.patch adds a function "addStmt" to ParentMap which actually adds a statement or updates the parent-children relations of an already existing statement. The comment to "addStmt" respects the remarks that some of you made at an earlier attempt ("A patch for printing policy and other stuff").

rewrite_options.patch adds two code rewrite options to the pretty printer of the clang frontend:
-rewrite-indent <value> Indentation of formatted rewrite output -rewrite-style <value> Style of formatted rewrite output: KR|ANSI

The ANSI style now works with the majority of statements and declarations with the exception of some ObjC constructs. I'm not sure whether there is a de facto standard for ObjC syntax or whether there are any syntactic restrictions in ObjC. Maybe someone familiar with ObjC can extend the pretty printer in order to respect the rewrite style for ObjC too.
However, there are some remarks necessary:
- A namespace declaration is always rewritten in KR mode. This is by intent.
- There are two other changes hidden in the patch: PrinterHelper::handledStmt got the current indentation level as an argument and I made Rewriter::getMappedOffset public. Both changes should be harmless.
- The IMHO most arguable point is the placement of the two options. They are now members of LangOptions, which is the pragmatic solution. Leaving them in PrintingPolicy would have caused a lot of refactoring during argument parsing (at that time there is no PrintingPolicy yet). Placing them in FrontendOptions (the more natural place) would have not only introduced more dependencies but also over-complicated the construction of PrintingPolicy.

I hope these patches make it into clang (my own source-to-source
transformer relies on them).

Best Olaf

rewrite_options.patch (16.9 KB)

parentmap.patch (1.03 KB)

I've applied the ParentMap patch here:

For the second patch, I really don't think we should be adding these fields to LangOptions. I understand the convenience for putting them there, but LangOptions shouldn't be some general object that we stuff random flags into because it is the most convenient context object. It's fundamentally a layering violation as well. LangOptions is in libBasic, while these flags only apply to an implementation detail of libAST. If we need a new context object, maybe we should do that, or possibly add these to ASTContext.

Hi @clang,

I've applied the ParentMap patch here:

Thank you, Ted.

For the second patch, I really don't think we should be adding these fields to LangOptions.

Yeah, actually that's my opinion too.

If we need a new context object, maybe we should do that, or possibly add these to ASTContext.

Well, we could use the PrintingPolicy itself furthermore. Things I don't like with this approach:
- PrintingPolicy mixes constant properties (e.g. Dump is set programmatically), parametric properties (e.g. Intendation) and variable properties (e.g. SuppressSpecifiers). Hmm, that's how things are already now. Hence I actually can live with that. But I still don't like it.
- The parameters have to be transported from the CompilerInvocation to the PrintingPolicy through the construction of the ASTContext. This means another argument in the ASTContext c'tor. I would prefer FrontendOpts. The only other way is the introduction of a new unit (RewriteOpts or something like this) in CompilerInvocation. This in turn means either an extended PrintingPolicy ctor or "initialization-by-assignment" in the ASTContext ctor. Extending the
PrintingPolicy ctor (which I'm generally not disinclined) raises the question how we deal with the related FIXME remarks in CFG and GRExprEngine.

Any other suggestions?
Best Olaf