On 23 January 2017 at 19:32, Adam Jackson <a...@redhat.com> wrote:
> Nobody who is using this functionality is ever not specifying a major
> version, which makes sense. If you don't care about a minor version,
> that's equivalent to saying you require minor >= 0, so just say so;
> likewise patch level.
>
> Likewise nobody using this functionality is ever not specifying an ABI
> class.
>
From a person how's native language extensively uses double negatives
- the ones used above are a bit confusing.
> Signed-off-by: Adam Jackson <a...@redhat.com>
> ---
> hw/xfree86/common/xf86Module.h | 6 ------
> hw/xfree86/doc/ddxDesign.xml | 20 +++++++++---------
> hw/xfree86/loader/loadmod.c | 47
> +++++++++++++++++-------------------------
> 3 files changed, 29 insertions(+), 44 deletions(-)
>
> diff --git a/hw/xfree86/common/xf86Module.h b/hw/xfree86/common/xf86Module.h
> index ff0e23e..faea07f 100644
> --- a/hw/xfree86/common/xf86Module.h
> +++ b/hw/xfree86/common/xf86Module.h
> @@ -141,12 +141,6 @@ typedef struct {
> const char *moduleclass; /* module class */
> } XF86ModReqInfo;
>
> -/* values to indicate unspecified fields in XF86ModReqInfo. */
> -#define MAJOR_UNSPEC 0xFF
> -#define MINOR_UNSPEC 0xFF
> -#define PATCH_UNSPEC 0xFFFF
> -#define ABI_VERS_UNSPEC 0xFFFFFFFF
> -
> #define MODULE_VERSION_NUMERIC(maj, min, patch) \
> ((((maj) & 0xFF) << 24) | (((min) & 0xFF) << 16) | (patch & 0xFFFF))
> #define GET_MODULE_MAJOR_VERSION(vers) (((vers) >> 24) & 0xFF)
> diff --git a/hw/xfree86/doc/ddxDesign.xml b/hw/xfree86/doc/ddxDesign.xml
> index 2ff2894..05ee042 100644
> --- a/hw/xfree86/doc/ddxDesign.xml
> +++ b/hw/xfree86/doc/ddxDesign.xml
> @@ -5293,12 +5293,12 @@ XFree86 common layer.
> as follows:
> <programlisting>
> typedef struct {
> - CARD8 majorversion; /* MAJOR_UNSPEC */
> - CARD8 minorversion; /* MINOR_UNSPEC */
> - CARD16 patchlevel; /* PATCH_UNSPEC */
> - const char * abiclass; /* ABI_CLASS_NONE */
> - CARD32 abiversion; /* ABI_VERS_UNSPEC */
> - const char * moduleclass; /* MOD_CLASS_NONE */
> + CARD8 majorversion;
> + CARD8 minorversion;
> + CARD16 patchlevel;
> + const char * abiclass;
> + CARD32 abiversion;
> + const char * moduleclass;
> } XF86ModReqInfo;
> </programlisting>
>
> @@ -5323,8 +5323,8 @@ typedef struct {
> The module's minor version must be
> no less than this value. This
> comparison is only made if
> - <structfield>majorversion</structfield> is
> - specified and matches.
> + <structfield>majorversion</structfield>
> + matches.
> </para></listitem></varlistentry>
>
> <varlistentry>
> @@ -5333,8 +5333,8 @@ typedef struct {
> The module's patchlevel must be no
> less than this value. This comparison
> is only made if
> - <structfield>minorversion</structfield> is
> - specified and matches.
> + <structfield>minorversion</structfield>
> + matches.
> </para></listitem></varlistentry>
>
> <varlistentry>
> diff --git a/hw/xfree86/loader/loadmod.c b/hw/xfree86/loader/loadmod.c
> index 85689be..528cc88 100644
> --- a/hw/xfree86/loader/loadmod.c
> +++ b/hw/xfree86/loader/loadmod.c
> @@ -617,32 +617,24 @@ CheckVersion(const char *module, XF86ModuleVersionInfo
> * data,
>
> /* Check against requirements that the caller has specified */
> if (req) {
> - if (req->majorversion != MAJOR_UNSPEC) {
> - if (data->majorversion != req->majorversion) {
> - LogMessageVerb(X_WARNING, 2, "%s: module major version (%d) "
> - "doesn't match required major version (%d)\n",
> - module, data->majorversion,
> req->majorversion);
> - return FALSE;
> - }
> - else if (req->minorversion != MINOR_UNSPEC) {
> - if (data->minorversion < req->minorversion) {
> - LogMessageVerb(X_WARNING, 2, "%s: module minor version "
> - "(%d) is less than the required minor "
> - "version (%d)\n", module,
> - data->minorversion, req->minorversion);
> - return FALSE;
> - }
> - else if (data->minorversion == req->minorversion &&
> - req->patchlevel != PATCH_UNSPEC) {
> - if (data->patchlevel < req->patchlevel) {
> - LogMessageVerb(X_WARNING, 2, "%sL module patch level
> "
> - "(%d) is less than the required patch
> "
> - "level (%d)\n", module,
> data->patchlevel,
> - req->patchlevel);
> - return FALSE;
> - }
> - }
> - }
> + if (data->majorversion != req->majorversion) {
> + LogMessageVerb(X_WARNING, 2, "%s: module major version (%d) "
> + "doesn't match required major version (%d)\n",
> + module, data->majorversion, req->majorversion);
> + return FALSE;
> + }
> + else if (data->minorversion < req->minorversion) {
> + LogMessageVerb(X_WARNING, 2, "%s: module minor version (%d) is "
> + "less than the required minor version (%d)\n",
> + module, data->minorversion, req->minorversion);
> + return FALSE;
> + }
> + else if (data->minorversion == req->minorversion &&
The above three can get a s/else if/if/.
But even without it (or the commit nitpick) things look a lot better.
Reviewed-by: Emil Velikov <emil.l.veli...@gmail.com>
-Emil
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
