curdeius added a comment.

I like these changes.
I have mixed feelings about `isCpp()` & co.
As @MyDeveloperDay said, I'd like it mean C++ only. I find it confusing that it 
means C++ or ObjC (even if the latter is a superset of the former). I'd rather 
see it spelt as `isCppOrObjC()` even if it's verbose but at least removes all 
confusion IMO.

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(); }
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()`.

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; }
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`?


cfe-commits mailing list

Reply via email to