MyDeveloperDay added a comment.
> The cost is financial, as it's developer time, which costs real money to
> companies. In the end, to support this, people like myself who are doing this
> as part of their job spend time that they'd otherwise spend to make other
> things better that might be more important.
Don't get me wrong I totally appreciate what you do,
But what you mean is it costs your employer. That I understand, but not all of
us are doing this on behalf of a company (more specially not yours), so you
could also say that your employer benefits the other way from those
contributors who give their time without them having to spend a dime.
I guess my question would be, should the cost to your employer really come into
the decision about whether something goes in or not? Other than of course we
are totally grateful to them for giving us so much of your time, but that
shouldn't have impact on what is worthy to go in should it? or am I wrong?
================
Comment at: lib/Format/TokenAnnotator.cpp:2546-2560
return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
(Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
- (Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for, tok::kw_while,
- tok::kw_switch, tok::kw_case, TT_ForEachMacro,
- TT_ObjCForIn) ||
- Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
- (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
- tok::kw_new, tok::kw_delete) &&
- (!Left.Previous || Left.Previous->isNot(tok::period))))) ||
- (Style.SpaceBeforeParens == FormatStyle::SBPO_Always &&
- (Left.is(tok::identifier) || Left.isFunctionLikeKeyword() ||
- Left.is(tok::r_paren) ||
- (Left.is(tok::r_square) && Left.MatchingParen &&
- Left.MatchingParen->is(TT_LambdaLSquare))) &&
- Line.Type != LT_PreprocessorDirective);
+ ((Left.isOneOf(tok::kw_if, tok::pp_elif, tok::kw_for,
tok::kw_while,
+ tok::kw_switch, tok::kw_case, TT_ForEachMacro,
+ TT_ObjCForIn) ||
+ Left.endsSequence(tok::kw_constexpr, tok::kw_if) ||
+ (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
----------------
klimek wrote:
> reuk wrote:
> > klimek wrote:
> > > I'm really confused about the changes os parens. It seems like this
> > > should just change the spaceRequiredBeforeParens(...), but instead seems
> > > to change parens in a way that completely changes the logic? (or
> > > alternatively is phabricator showing this wrong?)
> > I think this looks like a bigger change than it really is because I added
> > an open-parens on line 2548, which then affected the formatting of the
> > following lines. I also added the `isSimpleTypeSpecifier` check, so that
> > functional-style casts (`int (foo)`) are given the correct spacing.
> a) why did you add the one in 2548? you didn't change any of the logic, right?
> b) there are 3 extra closing parens in 2560, I see one more new opening in
> 2556, but that also seems superfluous?
> One question is whether we should pull this apart into bools with names that
> make sense :)
Isn't this just a classic example of where rewriting this as a series of static
functions e.g.
```
static bool someBehavior(Line, Left)
{
if (Line.Type == LT_ObjCDecl)
return true;
if (Left.is(tok::semi)
return true;
......
return false;
}
```
would be so much easier to understand?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55170/new/
https://reviews.llvm.org/D55170
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
