[PATCH] D117520: [clang-format] Fix SeparateDefinitionBlocks issues

[ksyx via Phabricator via cfe-commits]

ksyx added inline comments.


================
Comment at: clang/lib/Format/DefinitionBlockSeparator.cpp:134
+          OperateIndex + 1 < Lines.size()) {
+        // UnwrappedLineParser's recognition of free-standing macro like
+        // Q_OBJECT may also recognize some uppercased type names that may be
----------------
HazardyKnusperkeks wrote:
> Shouldn't we set a type for such cases instead of repeating the detection code here?
Here I actually did a few more checks to limit the impact to the minimum but I am happy to do that if that's necessary.


================
Comment at: clang/lib/Format/DefinitionBlockSeparator.cpp:135
+        // UnwrappedLineParser's recognition of free-standing macro like
+        // Q_OBJECT may also recognize some uppercased type names that may be
+        // used as return type as that kind of macros, which is a bit hard to
----------------
HazardyKnusperkeks wrote:
> As a Qt user who also wants to use your patch, please add a test for that case. ;)
For `Q_OBJECT` if used as pattern like this:
```
class X : ... {
  Q_OBJECT
public:
  // ...
}
```
I think this patch has no effect on this? The comment here is just a repetition of that in unwrapped parser


================
Comment at: clang/unittests/Format/DefinitionBlockSeparatorTest.cpp:143
+              "\n"
+              "  LONGTYPENAME\n"
+              "  Foobar(int t, int p) {\n"
----------------
HazardyKnusperkeks wrote:
> Maybe really use HRESULT? People will know what that should be.
It actually does not matter what type name it is as long as it is an identifier with >=5 characters and all uppercased


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117520/new/

https://reviews.llvm.org/D117520