[PATCH] D80079: [clang-format] [NFC] isCpp() is inconsistently used to mean both C++ and Objective C, add language specific isXXX() functions

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: clang/include/clang/Format/Format.h:1632
+  bool isCppOnly() const { return Language == LK_Cpp; }
+  bool isObjectiveC() const { return Language == LK_ObjC; }
+  bool isCpp() const { return isCppOnly() || isObjectiveC(); }
----------------
curdeius wrote:
> sammccall wrote:
> > Just my 2c - I find the current meaning of isCpp easier to understand, and would prefer isObjectiveC to mean objective-C/C++. h if it exists.
> > 
> > Reasons:
> >  - this is consistent with LangOptions::CPlusPlus and LangOptions::ObjC
> >  - when checking for C++, also applying these rules to ObjC++ should be the common/default case, and excluding ObjC++ the special case that justifies more precise syntax (and honestly, I'd find `isCpp && !isObjC` to carry the clearest intent in that case). IOW, this seems like it will attract bugs.
> > 
> > > perhaps a better name for isCpp() is isCStyleLanguages()
> > 
> > Clang uses the term "C family languages", and this includes C, C++, ObjC, ObjC++.
> > If you really want to avoid the conflict between C++ the boolean language option and C++ the precise language mode, I'd suggest `isPlusPlus()` and `isObjective()`. But I think consistency with LangOptions is worth more.
> I'd rather go for coherence with `LanguageKind` options and spell it `isObjC()`.
Peanut gallery says: Please be consistent! If you're going to do `isCppOrObjC()`, then you should also do `isObjC()`, not `isObjectiveC()`. (Or vice versa.)

What about Objective-C++? Are people using `isCppOrObjectiveC()` to mean "well actually it's Objective-C++ but we have no LanguageKind for that, oops?"

Re the term "C family languages," I've heard "curly-brace languages" — but that would include C#, Java, and JavaScript as well.

It seems to be that the members should be
```
bool isC() const { return Language == LK_Cpp; }  // no LK_C yet
bool isCpp() const { return Language == LK_Cpp; }
bool isObjectiveC() const { return Language == LK_ObjectiveC; }
bool isObjectiveCpp() const { return Language == LK_ObjectiveC; }  // no LK_ObjectiveCpp yet
bool includesCpp() const { return isCpp() || is ObjectiveCpp(); }
```
and that most of the callers you're talking about should be using `includesCpp()` instead of `isCpp()`.


================
Comment at: clang/include/clang/Format/Format.h:1635
   bool isCSharp() const { return Language == LK_CSharp; }
+  bool isProtoBuf() const { return Language == LK_Proto; }
+  bool isTableGen() const { return Language == LK_TableGen; }
----------------
curdeius wrote:
> sammccall wrote:
> > These functions that don't *even in principle* do more than compare to an enum seem like extra indirection that hurts understanding of the code (have to look up what isObjectiveC() does, or have subtle bugs).
> > 
> > I suspect isCSharp() was added due to a misunderstanding of what isCpp() was for.
> Ditto, maybe `isProto` and `isTextProto`?
If the name of the language is "Protobuf", then the name of the function should be `isProtobuf()`.


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

https://reviews.llvm.org/D80079