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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> 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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]