Re: [PATCH spice-streaming-agent 1/2] mjpeg plugin: split off the declaration into a header

[Christophe de Dinechin <cdupontd@xxxxxxxxxx>]

> 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.

Also, it is weird is to include <config.h> and not “config.h”… It’s a project header, not system header.


Thanks
Christophe


> 
>>> @@ -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