MyDeveloperDay marked 2 inline comments as done.
MyDeveloperDay added a comment.
I feel like there might something of a concencus forming.. If I take the time
to redo following the suggestions @sammccall do you think you could live with
it?
================
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(); }
----------------
Quuxplusone wrote:
> 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()`.
So I think I'd refrain from adding the `isC()` and `isObjectiveCpp()` unless
they were added to clang itself. But if they were then I think I'd be 100% with
you.
I'm not really a fan of the IsCppOnly() this was really to minimize the initial
changes to use a function, I actually think 'isCppOrObjC()' is a better choice,
but I think I'd do that transistion from `isCpp -> isCppOrObjC()` and
`isCppOnly() -> isCpp()` in a separate revision.
================
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; }
----------------
Quuxplusone wrote:
> 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()`.
I'm good with changing it to use the correct for obj `isObjC()` and `isProto()`
annd `isTextProto()`
I don't really mind what we call them I just don't like the extra visual
complexity that the `Style.Langauge == LK_XXX` and `Style.Langauge != LK_XXX`
brings
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
