patch to preserve carriage returns when using clang-format VS plugin

[Manuel Klimek]

Looks good - do you need me to submit it or do you have submit access?

Cheers,
/Manuel


On Thu, Nov 28, 2013 at 12:56 PM, jpark37 . <jpark37 at gmail.com> wrote:

> I had different size types to try to match the function signature of
> find_first_of in StringRef.cpp, which is actually different from the
> signature in StringRef.h. (This inconsistency should probably be fixed at
> some point.) I've fixed up the patch, retested, and attached.
>
> As Manuel said, the escaping cases aren't going to expand beyond what's
> needed for newlines.
>
>
> On Thu, Nov 28, 2013 at 6:53 AM, Manuel Klimek <klimek at google.com> wrote:
>
>> On Thu, Nov 28, 2013 at 12:32 PM, Alp Toker <alp at nuanti.com> wrote:
>>
>>> So, I think XML is just the wrong format to use here. There have been
>>> various XML implementations in LLVM and they all started like this, then
>>> went downhill as the need for more entity escaping rules came up until
>>> finally getting removed.
>>>
>>> clang-format deals primarily with whitespace, and that's ironically one
>>> of the hardest things to preserve effectively in XML.
>>>
>>> How about getting clang-format to generate an edit script? They're dead
>>> simple to generate in C++, easy to parse in C#, and can be applied with
>>> standard tools like ed or diffutils for testing. No escaping needed.
>>>
>>> Alternatively, could expose a libFormat entry point, deploy as a DLL and
>>> P/Invoke it directly from the VS extension.
>>>
>>> I can help out with either of these -- let's put an end to "XML"
>>> implementations in clang :-)
>>>
>>
>> I would also have preferred to not to produce XML in clang-format. Note
>> that it was not done for VS, but for the Eclipse integration.
>>
>> I read up on the XML spec, and whitespace doesn't seem hard to preserve
>> here - an XML implementation must provide all whitespace as-is; the one
>> problem is that the XML spec actually enforces normalization of newlines to
>> \n before parsing, which makes it look like this is the only XML specific
>> thing we need.
>>
>> Cheers,
>> /Manuel
>>
>>
>>>
>>> Alp.
>>>
>>>
>>>
>>>
>>> On 28/11/2013 10:36, Manuel Klimek wrote:
>>>
>>>> Index: tools/clang-format/ClangFormat.cpp
>>>> ===================================================================
>>>> --- tools/clang-format/ClangFormat.cpp  (revision 195826)
>>>> +++ tools/clang-format/ClangFormat.cpp  (working copy)
>>>> @@ -173,6 +173,27 @@
>>>>    return false;
>>>>  }
>>>> +static void outputReplacementXML(StringRef Text) {
>>>> +  const char *Data = Text.data();
>>>>
>>>> There's usually no need to go back to raw char *'s when you have
>>>> StringRef's (they kinda replace raw char *s).
>>>>
>>>> +  size_t From = 0;
>>>>
>>>> Any reason to have different types for From and Index?
>>>>
>>>> +  StringRef::size_type Index;
>>>> +  while ((Index = Text.find_first_of("\n\r", From)) !=
>>>> StringRef::npos) {
>>>> +    llvm::outs().write(Data + From, Index - From);
>>>>
>>>> llvm::outs() << Text.substr(From, Index - From);
>>>>
>>>> +    switch (Data[Index]) {
>>>> +    case '\n':
>>>> +      llvm::outs() << "
";
>>>> +      break;
>>>> +    case '\r':
>>>> +      llvm::outs() << "
";
>>>> +      break;
>>>> +    default:
>>>> +      llvm::errs() << "error: unexpected character encountered\n";
>>>>
>>>> As this would be a logic error, I'd use llvm_unreachable(...);
>>>>
>>>> +    }
>>>> +    From = Index + 1;
>>>> +  }
>>>> +  llvm::outs().write(Data + From, Text.size() - From);
>>>>
>>>> llvm::outs() << Text.substr(From);
>>>>
>>>> +}
>>>> +
>>>>  // Returns true on error.
>>>>  static bool format(StringRef FileName) {
>>>>    FileManager Files((FileSystemOptions()));
>>>> @@ -205,8 +226,9 @@
>>>>           I != E; ++I) {
>>>>        llvm::outs() << "<replacement "
>>>>                     << "offset='" << I->getOffset() << "' "
>>>> -                   << "length='" << I->getLength() << "'>"
>>>> -                   << I->getReplacementText() << "</replacement>\n";
>>>> +                   << "length='" << I->getLength() << "'>";
>>>> +      outputReplacementXML(I->getReplacementText());
>>>> +      llvm::outs() << "</replacement>\n";
>>>>      }
>>>>      llvm::outs() << "</replacements>\n";
>>>>    } else {
>>>>
>>>>
>>>>
>>>> On Thu, Nov 28, 2013 at 12:42 AM, jpark37 . <jpark37 at gmail.com <mailto:
>>>> jpark37 at gmail.com>> wrote:
>>>>
>>>>     Oops, sorry; the attached patch is updated and retested. I ran
>>>>     clang-format, and it created more diffs than just my changes;
>>>>     those have been undone to keep the patch focused. I've also
>>>>     switched the cascading if to a switch statement.
>>>>
>>>>     - James
>>>>
>>>>
>>>>     On Wed, Nov 27, 2013 at 5:24 PM, Daniel Jasper <djasper at google.com
>>>>     <mailto:djasper at google.com>> wrote:
>>>>
>>>>         I'd like Manuel to take a look, but in general, please format
>>>>         Clang/LLVM files with the correct style (i.e. "clang-format
>>>>         -style LLVM") :-).
>>>>
>>>>
>>>>         On Wed, Nov 27, 2013 at 11:28 AM, jpark37 . <jpark37 at gmail.com
>>>>         <mailto:jpark37 at gmail.com>> wrote:
>>>>
>>>>             Hello there,
>>>>
>>>>             I'm seeing newlines without carriage returns when using
>>>>             the clang-format plugin for Visual Studio. The issue seems
>>>>             to be that clang-format is not escaping newline characters
>>>>             when run with -output-replacements-xml, so the .NET XML
>>>>             stuff ends up collapsing \r\n down to \n. I've attached a
>>>>             patch that I've tested and appears to address the problem.
>>>>
>>>>             - James
>>>>
>>>>             _______________________________________________
>>>>             cfe-commits mailing list
>>>>             cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>>>>             http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>
>>>
>>> --
>>> http://www.nuanti.com
>>> the browser experts
>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131128/1e4efbeb/attachment.html>