sammccall 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(); } ---------------- 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. ================ 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; } ---------------- 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80079/new/ https://reviews.llvm.org/D80079 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits