On Wed, Jul 13, 2011 at 11:21 AM, Colin McCabe <cmccabe@xxxxxxxxxxxxxx> wrote:
> On Wed, Jul 13, 2011 at 10:52 AM, Yehuda Sadeh Weinraub
> <yehudasa@xxxxxxxxx> wrote:
>> Following are my remarks to the new coding style document:
>>
>> On Wed, Jul 13, 2011 at 10:34 AM, Yehuda Sadeh Weinraub
>> <yehudasa@xxxxxxxxx> wrote:
>>> - for regular classes, CamelCaps, private: section, etc.
>>>
>>> * Naming > Variable Names:
>>>
>>> Google uses _ suffix for class members. We haven't up until now. Should we?
>>
>> No.
>
> Quick quiz: line 144 of rgw/rgw_op.cc, is ret a local or class
> variable? How about line 1340? Or line 1419?
> I am on commit 4761317b754c6a903862d1913b53c8ab5e06b8eb .
Excuse me for not playing along.
>
> It's just confusing for class variables to teleport in with the same
> names used by local variables elsewhere in the file. If you are lucky,
> you'll get a type mismatch, but what if you're not? The fact that
> local variables shadow class variables is "obvious" to someone with 10
> years of C++ experience, but not so much to people from the community
> trying to read the code.
At least they don't throw up when seeing the trailing underscore.
>
> This is why languages like Python don't have the "implicit this"--
> every access to a class variable in python must look like
> self.variable.
> We should follow Google's lead here.
I disagree. Unless we modify all our existing code to signify class
member variables, adding some marker will just add to the confusion
(and if any, I'd rather have m_ prefix than a trailing underscore, but
that's a different issue) as we'd end up with mixed code. In most
cases it's pretty clear what the variables scope is, don't clutter the
entire code for that.
>
>>
>>>
>>> * Naming > Constant Names:
>>>
>>> Google uses kSomeThing for constants. We prefere SOME_THING.
>>>
>>> * Naming > Function Names:
>>>
>>> Google uses CamelCaps. We use_function_names_with_underscores().
>>>
>>> Accessors are the same, {get,set}_field().
>>>
>>> * Naming > Enumerator Names:
>>>
>>> Name them like constants, as above (SOME_THING).
>>>
>>> * Comments > File Comments:
>>>
>>> Don't sweat it, unless the license varies from that of the project (LGPL2) or
>>> the code origin isn't reflected by the git history.
>>>
>>> * Formatting > Conditionals:
>>>
>>> - No spaces inside conditionals please, e.g.
>>>
>>> if (foo) { // okay
>>>
>>> if ( foo ) { // no
>>>
>>> - Always use newline following if:
>>>
>>> if (foo)
>>> bar; // okay
>>>
>>> if (foo) bar; // no, usually hardler to visually parse
>>>
>>>
>>>
>>>
>>> The following guidelines have not been followed in the legacy code,
>>> but are worth mentioning and should be followed strictly for new code:
>>>
>>> * Header Files > Function Parameter Ordering:
>>>
>>> Inputs, then outputs.
>>
>> I generally agree, just that there's a class of strcpy like functions
>> that put the destination first. This usually makes sense when you have
>> multiple functions that do similar things with varied input
>> parameters:
>>
>> char *strcpy(char *dest, const char *src);
>>
>> char *strncpy(char *dest, const char *src, size_t n);
>>
>> So in both cases, dest and src are the first and second parameters,
>> which looks more consistent.
>
> Well, nothing is set in stone. There are always exceptions if it
> really makes sense. If you are designing a function patterned after
> strcpy or whatever, making it "look like" strcpy probably makes a lot
> of sense.
>
>>
>> And there's also the case of using default values, which usually
>> (though not always) applies to the input parameters. Enforcing strict
>> input before output ordering wouldn't work there.
>
> Well, the Google document says not to use default parameters because
> "they obscure the interface."
> So that is a non-issue.
>
> Check out commit 6e2b594b3329cb08bf54cd325776b236f2e6c4f1 for an
> example of how default parameters can really bite.
>
The same goes here. Default parameters *may* bite, yes. But they're
also an important tool that can ease up development and make code look
much more concise and readable. As usual, I don't think that
restricting your use of the language because of potential errors is
the better idea. And please, please don't add a compile flag that'll
prevent it.
Yehuda
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
