[cfe-dev] clang-format: confused C style cast in template, moving AnnotatingParser and add unit tests?

[Jean-philippe Dufraigne]

Thanks for you feedback.
I agree, the test i've written allow to make regression tests for the
issues.

Testing AnnotatingParser would have removed formatting consideration from
the parser tests which should have produced simpler and more targeted and
stable tests for it.
Since the formatting is dependent on its output, separating these 2
considerations seemed to make sense.

If you think that it may be worthwhile, I could try to code a small working
example of test for a stand alone AnnotatingParser to see whether it would
be of use to you.
Otherwise I'll carry on trying to fix these issues.

Best Regards,
JP

2014-09-21 20:12 GMT+01:00 Daniel Jasper <djasper at google.com>:

>
>
> On Sun, Sep 21, 2014 at 7:33 PM, Jean-philippe Dufraigne <
> j.dufraigne at gmail.com> wrote:
>
>> Hello Daniel,
>>
>> I was looking at a formatting issue with "std::function<void(int, int)>
>> callback;". "(int, int)" is understood as a C-style cast and so the spacing
>> follow this understanding.
>>
>> While trying to address this, I found that the existing test for member
>> function reference qualifier relied on the confusion. it results in
>> inconsistent format when a name is added thought (see last 2 lines of my
>> test function)
>>
>> This is not really dependent on the different style, and it would seem
>> unit test for AnnotatingParser would help when updating it.
>>
>> Would it make sense to move AnnotatingParser in its own files and then
>> start adding unit tests for it?
>>
>
> I don't think so. I like the unit tests as they are, testing the format of
> small snippets of actual code. Directly testing the AnnotatingParser feels
> a bit like testing implementation details. Also, the test you have written
> shows that you can test exactly what you want to test with the existing
> test infrastructure, or am I missing something?
>
> I would think that we would want to split the class in its .h and .cpp
>> file.
>> I'd be happy to provide a patch for this, though reviewing this kind of
>> changes is often tricky as diffs can be unhelpful, do you have any
>> preference on how it is done?
>>
>> The test function that shows the issues:
>>
>> TEST_F(FormatTest, ConfigurableSpacesInParenthesesTryThings) {
>>   FormatStyle Spaces = getLLVMStyle();
>>
>>   Spaces.SpacesInParentheses = true;
>>   verifyFormat("void function( int, int );", Spaces);
>>   verifyFormat("std::function<void(int, int)> callback;", Spaces); //
>> inconsistent
>>   verifyFormat("#define x ( (int)-1 )", Spaces);
>>
>>   Spaces.SpacesInParentheses = false;
>>   Spaces.SpacesInCStyleCastParentheses = true;
>>   verifyFormat("void function(int, int);", Spaces);
>>   verifyFormat("std::function<void( int, int )> callback;", Spaces); //
>> inconsistent
>>   verifyFormat("#define x (( int )-1)", Spaces);
>>
>>   verifyFormat("Deleted &operator=(const Deleted &)& = default;");
>>   verifyFormat("Deleted &operator=(const Deleted &other) & = default;");
>> // inconsitent
>> }
>>
>> Thanks,
>> JP
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140921/47f78bde/attachment.html>