Page MenuHomePhabricator

[clang-format] Add space in placement new expression
AcceptedPublic

Authored by omarahmed on Jun 7 2022, 9:10 PM.

Details

Summary

Add AfterPlacementNew option to SpaceBeforeParensOptions to have more control on placement new expressions.

Fixes https://github.com/llvm/llvm-project/issues/41501
Relates to https://github.com/llvm/llvm-project/issues/54703

Diff Detail

Event Timeline

omarahmed created this revision.Jun 7 2022, 9:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 9:10 PM
omarahmed requested review of this revision.Jun 7 2022, 9:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2022, 9:10 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Apart from some missing tests, looks promising!

clang/unittests/Format/FormatTest.cpp
15312–15322

Are there any tests with AfterPlacementNew = false;? Could you add those please?

HazardyKnusperkeks added a project: Restricted Project.Jun 8 2022, 5:00 AM
clang/include/clang/Format/Format.h
3498

Please sort after AfterOver... here and all other occasions.

clang/lib/Format/Format.cpp
1332

This isn't needed, because the default CTor initializes it with true.
Or you change that, I don't know right now why all other attributes are initialized with false.

clang/unittests/Format/FormatTest.cpp
15314

Assert on that.

MyDeveloperDay added a comment.EditedJun 11 2022, 9:14 AM

Thank you for the patch, was there a github issue for this? Can we just validate that those requirements are covered here too while we are at it?

https://github.com/llvm/llvm-project/issues/54703
https://github.com/llvm/llvm-project/issues/41501 ( I know this is yours, but ensure you mention it in the summary!) so the commit gets tied to the review

I'll agree with @curdeius I think this looks on the right lines (thank you for doing the rst, so many of us forget to do that normally!) .. I don't have a problem with adding something like this I think it will help give people fine control

clang/lib/Format/TokenAnnotator.cpp
3399

what case is covered here by spaceRequiredBeforeParens(Right)?

clang/unittests/Format/FormatTest.cpp
15312

you should probably give yourself your own test, for SpacePlacementNew

MyDeveloperDay added inline comments.Jun 11 2022, 9:21 AM
clang/include/clang/Format/Format.h
3498

For new options they tend to go through a live cycle bool->Enum->Structure...

can I as we start with an enum not a bool and do something like Always,None,Leave so Leave will just leave the code completely as it is today..

We get a lot of criticism for changing the defaults, often we are not changing them we are fixing things or now we format where we never did before.

I think it helps (as a concession to those who want us to NEVER change anything), if new options can have a Leave option so people don't get one or the other it just leaves the code neither formatted with a space or without a space, but doing what clang-format did before.

i.e. in the past we've been asked to be able to turn off/on individual styles. by ensuring we always have a "Leave" option we can guard the changes and in theory not introduce a change in people current formatting when using a later binary.

3555–3557

swap

3566

move down one P comes after O AfterO -> AfterP

clang/lib/Format/Format.cpp
938

move down one line

MyDeveloperDay requested changes to this revision.Jun 11 2022, 9:21 AM
This revision now requires changes to proceed.Jun 11 2022, 9:21 AM
omarahmed edited the summary of this revision. (Show Details)Jun 11 2022, 11:04 AM
omarahmed added inline comments.Jun 12 2022, 6:36 AM
clang/lib/Format/Format.cpp
1332

I think they are initialized with false so that when we come to this case, it breaks while all of them are false. (I am not sure so) I will try to add a test to cover 'SBPO_Never' but after converting it to enum.

clang/lib/Format/TokenAnnotator.cpp
3399

I intended to cover this case: new (buf) T so basically placement new expressions, excluding the overloaded new operator function declarations/definitions like T *operator new(size_t size) {} and excluding function declarations/definitions named new like T *new();

clang/unittests/Format/FormatTest.cpp
15312

I was thinking of covering placement delete too and adding the tests for this patch to this test as it should add more cases to new/delete operators.

clang/lib/Format/Format.cpp
1332

Then you need to set it to false in the CTor.

curdeius edited the summary of this revision. (Show Details)Jun 14 2022, 11:09 PM

Does this patch really fix https://github.com/llvm/llvm-project/issues/54703?
If so, please add test for it. Otherwise remove the link from the summary (and if possible handle it in another review).

omarahmed updated this revision to Diff 437468.Jun 16 2022, 1:18 AM
omarahmed edited the summary of this revision. (Show Details)

Add coverage for placement delete expressions and transform bool option to enum

Does this patch really fix https://github.com/llvm/llvm-project/issues/54703?
If so, please add test for it. Otherwise remove the link from the summary (and if possible handle it in another review).

It should cover this case too, I have added a test to verify that it covers it.

I think you are missing a PARSE unit test

clang/docs/ClangFormatStyleOptions.rst
4261

Are you making this by hand or by running tools/docs/dump_format_style.py?

MyDeveloperDay added inline comments.Jun 16 2022, 3:37 AM
clang/include/clang/Format/Format.h
3495

should this be Always/Never

clang/lib/Format/Format.cpp
939

when you've fixed the code please mark the comment "done.."

clang/lib/Format/TokenAnnotator.cpp
3396

shouldn't the very first part of this be?

if (Style.SpaceBeforeParensOptions.AfterPlacementOperator != FormatStyle::SpaceBeforeParensCustom::APO_Leave)
{

....

}

i.e. don't we want a zero change if someone says "Leave"

clang/include/clang/Format/Format.h
3495

Nope, this should be removed. :)

3555–3557

As said in the other comment, there is code that depends on setting to never in the default constructor, so please do.

clang/unittests/Format/FormatTest.cpp
10142

Can this be valid code?

omarahmed marked 12 inline comments as done.Jun 17 2022, 1:36 AM
omarahmed added inline comments.Jun 17 2022, 2:32 AM
clang/docs/ClangFormatStyleOptions.rst
4261

I was doing it manually, as I didn't know there is a script that does that. I tried using the script but seems like it has a parsing error with this logic as it is the first time to use enum inside a struct in format.h file. So it seems that the parsing logic for this case needs to be added. I will try adding this parsing case to dump_format_style.py and add it to my next update for this patch.

omarahmed marked an inline comment as not done.

Refactor the tests and add new parsing logic for nested enums in dump_format_style.py

As I understand, the default behavior for when the user didn't use SBPO_Custom is to add a space in placement operators based on this issue. And, at the same time, the default behavior should be APO_Never when we have SBPO_Custom so that we handle other code that depends on that. the existing tests confirm this understanding. So, the current logic was added based on this understanding.

clang/lib/Format/TokenAnnotator.cpp
3396

The current code logic, when we do not enter this suggested condition, will switch to this condition. And will not leave the code as it is, but will change it.

I have thought of getting rid of this condition entirely, but it handles the default situation suggested here, not only for cpp but for other languages like js:

As I understand, the default behavior for when the user didn't use SBPO_Custom is to add a space in placement operators based on this issue. And, at the same time, the default behavior should be APO_Never when we have SBPO_Custom so that we handle other code that depends on that. the existing tests confirm this understanding. So, the current logic was added based on this understanding.

I tried to add an extra condition to this condition to support the main logic that we are pursuing. so that it would be like that:

if (Style.SpaceBeforeParensOptions.AfterPlacementOperator != FormatStyle::SpaceBeforeParensCustom::APO_Leave && Left.isOneOf(tok::kw_new, tok::kw_delete))

But that wouldn't fix the leave situation, as it will be also at the end, reach this part which will

return false

and force no space.

I think all of that was raised because all current "SpaceBeforeParanthesesOptions" are either true or false and this is the only enum

However, I completely agree with this logic which is to begin our checking for leave. But I think we should refactor all the "spaceBeforeParenthesesOptions" conditions to handle this and do that after all the options have been transformed to enums.

Add Parse check

omarahmed marked 5 inline comments as done and an inline comment as not done.Jun 17 2022, 11:28 AM

Nice work adding the capability to the dump script.

clang/docs/ClangFormatStyleOptions.rst
4264

This seems to be not entirely correct yet. :)

clang/include/clang/Format/Format.h
3504
clang/lib/Format/TokenAnnotator.cpp
3396

SpaceBeforeParensOptions should always be expanded, regardless of SpaceBeforeParens, as far as I remember. SpaceBeforeParens is only for the one configuring clang-format, the code that formats should always and only check SpaceBeforeParensOptions.

clang/unittests/Format/FormatTest.cpp
10145

Can this be valid code? If not we don't need to assert on how it is formatted, only that it doesn't crash.

clang/docs/tools/dump_format_style.py
336

Can you show us a screenshot of how these changes will look in HTML?

omarahmed updated this revision to Diff 438433.Jun 20 2022, 9:53 AM
  • Add version for nestedEnums and nestedFields
  • Make tests valid
clang/docs/tools/dump_format_style.py
336

omarahmed marked 3 inline comments as done and an inline comment as not done.Jun 20 2022, 10:02 AM
omarahmed added inline comments.
clang/lib/Format/TokenAnnotator.cpp
3396

But in general, doesn't that could create some kind of conflict like if someone used a specific option in SpaceBeforeParens and then defined inside SpaceBeforeParensOptions some rules that could conflict with SpaceBeforeParens , what should be the priority between them?

3396

In this specific case, for AfterPlacementOperator if someone doesn't define it, it should keep the default behavior in the past which is to add a space, and we defined the default to be APO_Never, so that's why I have conditioned it with Custom so that if someone doesn't want to define it, then we keep the old behavior.

MyDeveloperDay accepted this revision.Jun 21 2022, 2:02 AM

Thanks for the Screenshot, please remember to mark your comments as "Done" once you've addressed them. From what I can tell this looks good.

clang/include/clang/Format/Format.h
3555–3557

Why Never and not Leave? basically introducing one or the other as a default won't that causes changes for people who don't want to use this?

This revision is now accepted and ready to land.Jun 21 2022, 2:02 AM
omarahmed marked 3 inline comments as done.Jun 21 2022, 2:34 AM
omarahmed updated this revision to Diff 438657.Jun 21 2022, 5:32 AM

Change the default to APO_Leave

omarahmed added inline comments.Jun 21 2022, 5:39 AM
clang/include/clang/Format/Format.h
3555–3557

I think that's a good point. I changed that to APO_Leave and added APO_Never to the code that depends on it.

I was guarding the default behavior with SpaceBeforeParentheses: Custom condition but I think still if someone defined it to be custom and didn't specify this option, it should still be leave.

clang/lib/Format/Format.cpp
1169

The part that depends on it to be APO_Never to align with SBPO_Never option.

omarahmed updated this revision to Diff 438719.Jun 21 2022, 8:36 AM

Format files

clang/include/clang/Format/Format.h
3555–3557

We change the behavior here, I'm still not convinced we should do that. Although I'm a big supporter of the Leave option.

omarahmed updated this revision to Diff 438962.Jun 22 2022, 3:36 AM

Format files

I don't have push credentials so If everything is okay with the patch, can you push it for me. My email is omarpiratee2010@gmail.com

clang/include/clang/Format/Format.h
3555–3557

I am neutral to both cases, so I leave the choice to you as I am not experienced with clang-format users. Let me know which choice you think would be better.

curdeius accepted this revision.Thu, Jul 14, 8:29 AM
curdeius added inline comments.
clang/include/clang/Format/Format.h
3504

We have verbs: removes, add, leave. Si I think it should be "remove" here.

omarahmed updated this revision to Diff 445172.Fri, Jul 15, 7:00 PM

Change removes to remove