[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:
> curdeius wrote:
> > ksyx wrote:
> > > 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.
> > > Shouldn't we set a type for such cases instead of repeating the detection code here?
> > 
> > :+1:
> Thanks! Shouldn't we drop are radically change this comment?
I've checked this comment but found nothing to change, since it has nothing to do with the duplicated check but just referring to where created this problem?


================
Comment at: clang/unittests/Format/DefinitionBlockSeparatorTest.cpp:143
+              "\n"
+              "  LONGTYPENAME\n"
+              "  Foobar(int t, int p) {\n"
----------------
HazardyKnusperkeks wrote:
> ksyx wrote:
> > 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
> That I know, but HRESULT would look familiar to at least some people.
This makes sense! Thanks for the suggestion.


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

https://reviews.llvm.org/D117520