> On 30 Jan 2018, at 14:05, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote: > > 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. That is not at all what I am saying. I am only pointing out that your notion of what looks good may differ from Frediano’s. And in that case, I have a meta-rule which is that whoever wrote the code first gets to set the style that they like. And then, following that style is just being polite to that initial contributor. Also, it’s more efficient to not spend time recoding stuff just for style. IOW, If I submit a patch to the kernel, I follow the kernel style. If you send a patch for code I wrote, I expect you to follow my style, irrespective of whether you like my style or not. That being said, if Frediano likes your suggestions (and it seems like he does) then I’m all for it. > >> 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. For me, it should first be efficient. There is no C coding style that works well for C++. They are different languages. In addition, the C++ code we are adding does not specially interact with C at the moment. Also to be taken into consideration: I expect that the streaming agent code will have to integrate into other projects at some later point, e.g. connect to Qemu or to some Windows wrapper. I’m pretty sure you can’t follow both the C++ std::, Qemu and Windows coding styles simultaneously ;-) (https://github.com/qemu/qemu/blob/master/CODING_STYLE) > > 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. It enforces the syntactic aspects, but not the use of namespaces or naming choices. Those, however, are documented in the various coding styles pointed to by the clang-format documentation. Thanks Christophe > >> 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