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

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 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

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.

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