On Tue, 2018-01-30 at 11:50 +0100, Christophe de Dinechin wrote:
> Hi Lukáš,
>
>
> In the specific case of the streaming agent, I believe it matters
> for instant productivity that the code follow a style that does not
> require additional thinking on Frediano’s part. So if Frediano likes
> it, it’s fine by me, otherwise don’t care.
I don't really understand this notion. Are we in such a hurry we
disregard coding standards and just whack away? :D No offense,
Frediano, not meant personally at you, I mean in general it's almost
never worth it to write messy code for the sake of speed. I find the
issues I mentioned incoherent and want to agree with everybody on a
style to adhere to.
> Also, rather than invent a style, I’d rather adopt an standard coding
> style, e.g. Google’s. And then use clang-format to enforce all the
> machine-enforceable parts of it.
The problem is we already have a C coding style 'invented' and since
C++ is an extension of C (with a bunch of quirks), I think it should be
coherent if possible.
I should play with clang-format a bit, my experience with earlier
formatters for C/C++ was less than stellar, so I just dropped it. If it
can enforce or even lint our style, I'm all for it.
> Regards,
> Christophe
>
> PS: Some comments on your suggestions anyway…
>
> > On 29 Jan 2018, at 15:19, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote:
> >
> > Hello everybody,
> >
> > I'd like to discuss a few things about the coding style for C++ in
> > Spice (looking at the streaming agent atm).
> >
> > Trying to keep this short and concise.
> >
> >
> > 1. Method names
> > Currently the method names are in CamelCase throughout the
> > streaming
> > agent. Methods are basically functions attached to a class, I
> > suggest
> > we use snake_case to be consistent with the function names.
> >
> > It's rather confusing when you see a call like SomeObject(), which
> > looks like a constructor, but you actually find out it's a method
> > call
> > from another method of the same class.
>
> Naming a method with a name that can also be a class is always
> ambiguous, CamelCase or not. Is color() a method or an ctor?
> So DeCamelCaseIfication not a solution to that problem.
No, you are right, just trying to make the best out of it :(
> BTW, CamelCase is so frequent in C++ that it often can be used to
> identify
> code as being C++ as opposed to plain C. To wit: LLVM, WebKit, Qt,
> etc.
I know some projects do use CamelCase for methods, I just don't
understand it at all and hate it. No other languages I know do that.
I'm fine with smallCamelCase though. First letter makes all the
difference.
> >
> >
> > ;2. Namespace names
> > Although not standard (you may have different experience), usually
> > namespaces are lowercase in C++.
>
> By that token, so do classes (in all of the standard library).
> But it’s generally not true outside of the standard library.
Yes, as I said, just trying to make the best of it :(
> > Also, they are hierarchical, I suggest
> > we use that and in streaming agent we change the namespace like so:
> >
> > SpiceStreamingAgent -> spice::streamingagent
> >
> > or (imho better):
> > SpiceStreamingAgent -> spice::streaming_agent
> >
> > And stick to this scheme, i.e. lowercase and toplevel namespace
> > 'spice', inside it a namespace of the component.
> >
> Not against the idea, but two levels of namespace for
> 2000 LOCs seems a tad bit overkill…
LOC is irrelevant...
i) We can have other components in the future that can be in namespace
spice::other_component, it creates clear structure.
ii) It wouldn't matter much if this was an isolated binary, but given
there are plugins, I'd try to keep it as organized as possible.
> > 3. Namespace coding style
> >
> > a) Let's not use `using namespace ...` ever even in .cpp files (see
> > i.e. [1]). In streaming agent we have at the beginning of every
> > .cpp:
> >
> > using namespace std;
> > using namespace SpiceStreamingAgent;
>
> Again, 2000 lines of code, unlikely to grow much.
> Google’s rule applies to their mega-projects, but for the agent,
> I think that “using namespace” makes the code leaner.
Not sure if you refer to std or SpiceStreamingAgent here. Assuming it's
std, I still consider it a good practice and (big IMO) convention
people are used to.
> >
> > For namespace std, "std::" is not a long prefix, clearly expresses
> > the
> > identifier is from the standard library and AFAIK most C++ projects
> > use
> > it this way.
> >
> > For namespace SpiceStreamingAgent, I didn't even know it worked for
> > definition of symbols in the namespace. First time I see it, it is
> > very
> > unusual. see b).
> >
> > b) Let's keep the following coding style for namespaces, i.e. for
> > streaming agent:
> >
> > namespace spice {
> > namespace streaming_agent {
> >
> > THE_CODE
> >
> > }} // namespace spice::streaming_agent
>
> Not too enthusiastic about }}
You can split it across two lines, but in this specific case, I see no
reason not to save a line (or more if you go deeper on the namespaces).
The comment is more or less mandatory and then it's clear.
> >
> >
> > We should add the guidelines to the website next to the C coding
> > style,
> > but I have no intention to be exhaustive (see [1] for how long it
> > can
> > be), let's add important cases as they come up and just use common
> > sense, keep the style of the local code and codereview to keep
> > things
> > in check?
>
> Let’s first share our preference on existing styles to see if we
> agree on anything…
> As for me, I have a slight preference for the LLVM coding style, but
> I made
> modifications in my own clang-format files.
I can agree on LLVM and quite like it. The only problem, as I said, is
it's inconsistent with what we have in C.
Cheers,
Lukas
> Regards
> Christophe
>
>
> >
> > Lukas
> >
> >
> > [1] https://google.github.io/styleguide/cppguide.html#Namespaces
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel