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

[Dmitri Gribenko]

On Mon, Dec 17, 2012 at 8:02 PM, jahanian <fjahanian at apple.com> wrote:
>
> On Dec 17, 2012, at 9:44 AM, Dmitri Gribenko <gribozavr at gmail.com> wrote:
>
>> On Mon, Dec 17, 2012 at 7:13 PM, jahanian <fjahanian at apple.com> wrote:
>>> 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?
>>
>>
>> +      std::string StringDecl = Declaration.str();
>> +      StringDecl += ";";
>>
>> Adding this semicolon should be the job of the pretty-printer.  For
>> example, "namespace Foo {}" does not need a semicolon.  (Yes, I know
>> it in objc-only now, but it is the declprinter that needs to be fixed,
>> not this.)
>
> Semicolon is needed by the current formatter. I did not want to change the current look
> of the DeclPrinter for this purpose. If formatter can be changed to not require a semicolon,
> this will not be needed.

Why not change DeclPrinter?  If the language syntax requires a
semicolon, why shouldn't DeclPrinter, that prints AST back to the
high-level language, omit it?

>> -void CommentASTToXMLConverter::appendToResultWithXMLEscaping(StringRef S) {
>> +void CommentASTToXMLConverter::appendToResultWithXMLEscaping(StringRef S,
>> +                                                             bool SkipSemi) {
>>   for (StringRef::iterator I = S.begin(), E = S.end(); I != E; ++I) {
>>     const char C = *I;
>>     switch (C) {
>> @@ -1307,6 +1353,9 @@
>>       case '\'':
>>         Result << "'";
>>         break;
>> +      case ';':
>> +        if (SkipSemi)
>> +          break;
>>
>> As per above, adding a hack and then reversing is not a good approach,
>> thus "SkipSemi" does not belong here.
>>
>
> I need to remove the semicolon because formatter, rightly, leaves it there.

Why remove it if the language syntax requires it?

>> +void *cxcursor::createFormatContext() {
>> +  return new RewriterTestContext();
>> +}
>>
>> +void cxcursor::disposeFormatContext(void *formatContext) {
>> +  delete static_cast<RewriterTestContext*>(formatContext);
>> +}
>>
>> Do we need these functions?  Why can not its only user
>>
>> --- tools/libclang/CIndex.cpp (revision 170208)
>> +++ tools/libclang/CIndex.cpp (working copy)
>> @@ -61,6 +61,8 @@
>>   D->StringPool = createCXStringPool();
>>   D->Diagnostics = 0;
>>   D->OverridenCursorsPool = createOverridenCXCursorsPool();
>> +  D->FormatContext = createFormatContext();
>>
>> just call new/delete directly?
>
> I am following the convention for similar use cases. But, it is trivial to do.

Well, there should be a reason for that (I don't see it).  If there
isn't -- those other functions are not needed, too.

Dmitri

-- 
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/