patch to preserve carriage returns when using clang-format VS plugin
James Park
jpark37 at gmail.com
Thu Nov 28 10:47:10 PST 2013
I do need this submitted for me. Thanks Manuel.
- James
On Thu, Nov 28, 2013 at 7:45 AM, Manuel Klimek <klimek at google.com> wrote:
> 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/8db44927/attachment.html>
More information about the cfe-commits
mailing list