> 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? > 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 _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel