> > > On 31 Jan 2018, at 12:26, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote: > > > >>> > >>> On 30 Jan 2018, at 17:54, Frediano Ziglio <fziglio@xxxxxxxxxx> wrote: > >>> > >>>>> > >>>>> On 30 Jan 2018, at 13:29, Lukáš Hrázký <lhrazky@xxxxxxxxxx> wrote: > >>>>> > >>>>> On Tue, 2018-01-30 at 06:32 -0500, Frediano Ziglio wrote: > >>>>>>> > >>>>>>> So that the plugin can later be included in a unit test. > >>>>>>> > >>>>>>> Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx> > >>>>>>> --- > >>>>>>> src/mjpeg-fallback.cpp | 26 ++++++-------------------- > >>>>>>> src/mjpeg-fallback.hpp | 34 ++++++++++++++++++++++++++++++++++ > >>>>>>> 2 files changed, 40 insertions(+), 20 deletions(-) > >>>>>>> create mode 100644 src/mjpeg-fallback.hpp > >>>>>>> > >>>>>>> diff --git a/src/mjpeg-fallback.cpp b/src/mjpeg-fallback.cpp > >>>>>>> index f41e68a..fac34bc 100644 > >>>>>>> --- a/src/mjpeg-fallback.cpp > >>>>>>> +++ b/src/mjpeg-fallback.cpp > >>>>>>> @@ -4,6 +4,8 @@ > >>>>>>> * Copyright 2017 Red Hat Inc. All rights reserved. > >>>>>>> */ > >>>>>>> > >>>>>>> +#include "mjpeg-fallback.hpp" > >>>>>>> + > >>>>>>> #include <config.h> > >>>>>>> #include <cstring> > >>>>>>> #include <exception> > >>>>>>> @@ -13,9 +15,6 @@ > >>>>>>> #include <syslog.h> > >>>>>>> #include <X11/Xlib.h> > >>>>>>> > >>>>>>> -#include <spice-streaming-agent/plugin.hpp> > >>>>>>> -#include <spice-streaming-agent/frame-capture.hpp> > >>>>>>> - > >>>>>>> #include "static-plugin.hpp" > >>>>>>> #include "jpeg.hpp" > >>>>>>> > >>>>>> > >>>>>> The include order looks weird. > >>>>>> Surely you want the config include before anything else as it could > >>>>>> change some system header behaviour. > >>>>>> But I think including mjpeg-fallback.hpp after the system headers > >>>>>> here is more coherent. > >>>>> > >>>>> This is an interesting topic :) > >>>>> > >>>>> The reason for including the header corresponding to a .cpp file first > >>>>> in it is that you will get undefined symbol errors if you are missing > >>>>> includes in the header which are present in the .cpp (and which you > >>>>> would put in front of it). This is described for example in the Google > >>>>> C++ Style Guide [1]. > >>>>> > >>>>> If there's something I should be aware of in C, please educate me :) > >>>> > >>>> +1 for Lukas’ rationale. > >>>> > >>>> LLVM enforces this order too, see > >>>> https://llvm.org/docs/CodingStandards.html#include-style. > >>>> It’s actually enforced by clang-format in that case. > >>>> > >>> > >>> I think I inherited the order from many time ago, is a good point. > >>> In spice-server I have a script that creates for each header a C file > >>> that > >>> include a single header just to test that the headers can be included. > >>> Would save me this manual job! Note that the spice-server style is > >>> different > >>> but this does not mean cannot be changed. > >>> > >>>> Also, it is weird is to include <config.h> and not “config.h”… It’s a > >>>> project > >>>> header, not system header. > >>>> > >>> > >>> I think this inherited too but you are right, should be "" syntax. > >>> > >>>> > >>>> Thanks > >>>> Christophe > >>>> > >>> > >>> About including config.h first: > >>> > >>> > >>> Short explanation: can save lot of pain. > >>> > >>> > >>> Long explanation follow! > >>> > >>> The config.h is generated by configure script for different reasons: > >>> - allow to configure some options; > >>> - detect available libraries; > >>> - detect portability issues. > >> > >> I understand. But if you use these config.h features in some header, > >> then it is my opinion that the header itself should include config.h. > >> Any reason why not? > >> > > > > One reason is that if the header needs to be installed on the system > > should not include config.h as config.h should be usually used by > > your program to avoid changing other program API/ABI. > > Another is that you are not sure the file is included ASAP. > There was some discussion off list, for instance was mentioned: https://www.gnu.org/software/autoconf/manual/autoconf-2.66/html_node/Configuration-Headers.html > If that’s the case, then your header is by definition not using config.h, yes > and can therefore be safely placed before config.h in the .cpp file. no, if your header include some system header (even recursively) the definitions could change. > I’d argue it should, since that allows you to check that some > config.h dependency did not mistakenly creep in. > yes, this would means including config.h at the beginning of all headers however as you don't want for public headers this would apply only to private ones that would seem a bit not coherent. > > > > Another question that come to my mind is why we use always "config.h" > > in the main directory instead for instance putting inside a > > include/my_program/config.h for example to be able to do a > > #include "my_program/config.h" with less conflicts possible. > > I think this is good practice, since it makes it possible to include your > code in a larger project, where the config.h may be generated at > a higher level (and selected by passing -I options). > The -I was mentioned also in autoconf manual linked above. They also mention that config.h is just the usual name. As we already have an include/spice-streaming-agent we could use that directory. For the sake of this single patch I would start the .cpp file with #include "config.h" #include "mjpeg-fallback.hpp" or #include <config.h> #include "mjpeg-fallback.hpp" maybe better for consistency with the actual code the second option. Frediano > > > > I think that many of these syntaxes came from examples and lot > > of historic copy&paste here and there. > > :-) > > > > >>> Now... porting Unix application can be really a pain. > >>> In config.h there can be options that change the declarations inside > >>> system headers! > >>> Just an example: _GNU_SOURCE. Enables some additional > >>> functions/structures/whatever specific to GNU. In some cases change > >>> the name of some structure fields. Now it may be possible that for > >>> instance your .hpp header include "string" which include "string.h" > >>> file which have some declaration added/removed. As you know headers > >>> are usually protected by some guards to avoid double/recursive > >>> inclusion. So including config.h after/before changes the available > >>> functions. > >>> So far so good, program won't compile and you can fix it. > >>> In my experience porting programs unfortunately what can happen is > >>> much worse. Some systems (really, I don't remember) export a different > >>> structure depending on some defines. What can happen is that structure > >>> "foo" in moduleA have some size/fields and others in moduleB. You link > >>> the two modules in the same executable library and when the program > >>> run you'll get some stack/heap corruption! Yes, this is a real case! > >>> Linux is quite developer friendly but fixing these header details in > >>> some nasty Unix environment is really a pain! > >>> Last year (2017) some people complained that C99 is too recent! > >>> I remember other cases where changing system include order make the > >>> same source crash. > >>> A case I remember are unixODBC headers where different defines > >>> make the header present 2 different ABI... but obviously the binary > >>> libraries you are linking expose just one, not hard to imagine the > >>> consequences running with the wrong defines. > >>> Yes, should not happen but really if to avoid mostly of these nasty > >>> issues you just have to include config.h first you do it. > >>> > >>> Frediano > >>> > >>>> > >>>>> > >>>>>>> @@ -41,11 +40,6 @@ static inline uint64_t get_time() > >>>>>>> } > >>>>>>> > >>>>>>> namespace { > >>>>>>> -struct MjpegSettings > >>>>>>> -{ > >>>>>>> - int fps; > >>>>>>> - int quality; > >>>>>>> -}; > >>>>>>> > >>>>>>> class MjpegFrameCapture final: public FrameCapture > >>>>>>> { > >>>>>>> @@ -69,18 +63,6 @@ private: > >>>>>>> uint64_t last_time = 0; > >>>>>>> }; > >>>>>>> > >>>>>>> -class MjpegPlugin final: public Plugin > >>>>>>> -{ > >>>>>>> -public: > >>>>>>> - FrameCapture *CreateCapture() override; > >>>>>>> - unsigned Rank() override; > >>>>>>> - void ParseOptions(const ConfigureOption *options); > >>>>>>> - SpiceVideoCodecType VideoCodecType() const { > >>>>>>> - return SPICE_VIDEO_CODEC_TYPE_MJPEG; > >>>>>>> - } > >>>>>>> -private: > >>>>>>> - MjpegSettings settings = { 10, 80 }; > >>>>>>> -}; > >>>>>>> } > >>>>>>> > >>>>>>> MjpegFrameCapture::MjpegFrameCapture(const MjpegSettings& > >>>>>>> settings): > >>>>>>> @@ -205,6 +187,10 @@ void MjpegPlugin::ParseOptions(const > >>>>>>> ConfigureOption > >>>>>>> *options) > >>>>>>> } > >>>>>>> } > >>>>>>> > >>>>>>> +SpiceVideoCodecType MjpegPlugin::VideoCodecType() const { > >>>>>>> + return SPICE_VIDEO_CODEC_TYPE_MJPEG; > >>>>>>> +} > >>>>>>> + > >>>>>>> static bool > >>>>>>> mjpeg_plugin_init(Agent* agent) > >>>>>>> { > >>>>>>> diff --git a/src/mjpeg-fallback.hpp b/src/mjpeg-fallback.hpp > >>>>>>> new file mode 100644 > >>>>>>> index 0000000..8044244 > >>>>>>> --- /dev/null > >>>>>>> +++ b/src/mjpeg-fallback.hpp > >>>>>>> @@ -0,0 +1,34 @@ > >>>>>>> +/* Plugin implementation for Mjpeg > >>>>>>> + * > >>>>>>> + * \copyright > >>>>>>> + * Copyright 2017 Red Hat Inc. All rights reserved. > >>>>>>> + */ > >>>>>>> +#ifndef SPICE_STREAMING_AGENT_MJPEG_FALLBACK_HPP > >>>>>>> +#define SPICE_STREAMING_AGENT_MJPEG_FALLBACK_HPP > >>>>>>> + > >>>>>>> +#include <spice-streaming-agent/plugin.hpp> > >>>>>>> +#include <spice-streaming-agent/frame-capture.hpp> > >>>>>>> + > >>>>>>> + > >>>>>>> +namespace SpiceStreamingAgent { > >>>>>>> + > >>>>>>> +struct MjpegSettings > >>>>>>> +{ > >>>>>>> + int fps; > >>>>>>> + int quality; > >>>>>>> +}; > >>>>>>> + > >>>>>>> +class MjpegPlugin final: public Plugin > >>>>>>> +{ > >>>>>>> +public: > >>>>>>> + FrameCapture *CreateCapture() override; > >>>>>>> + unsigned Rank() override; > >>>>>>> + void ParseOptions(const ConfigureOption *options); > >>>>>>> + SpiceVideoCodecType VideoCodecType() const; > >>>>>>> +private: > >>>>>>> + MjpegSettings settings = { 10, 80 }; > >>>>>>> +}; > >>>>>>> + > >>>>>>> +} // namespace SpiceStreamingAgent > >>>>>>> + > >>>>>>> +#endif // SPICE_STREAMING_AGENT_MJPEG_FALLBACK_HPP > >>>>>> > >>>>>> Frediano > >>>>> > >>>>> Lukas > >>>>> > >>>>> > >>>>> [1] > >>>>> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes > > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel