Bug 105429 - Patch: Fixes compiler warnings in lib
Summary: Patch: Fixes compiler warnings in lib
Status: RESOLVED FIXED
Alias: None
Product: libfprint
Classification: Unclassified
Component: libfprint (show other bugs)
Version: unspecified
Hardware: All All
: medium normal
Assignee: libfprint-bugs
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-10 18:23 UTC by Mark Harfouche
Modified: 2018-05-25 11:22 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
proposed patch (5.98 KB, patch)
2018-03-10 18:23 UTC, Mark Harfouche
Details | Splinter Review
Commented out unused static variables. (2.47 KB, patch)
2018-03-11 20:25 UTC, Mark Harfouche
Details | Splinter Review
Removes compiler warnings in morph.c (5.04 KB, patch)
2018-03-11 20:31 UTC, Mark Harfouche
Details | Splinter Review
[Patch] lib: Fixes compilation warning in aes2501 (1.21 KB, patch)
2018-03-11 20:42 UTC, Mark Harfouche
Details | Splinter Review
[Patch] lib: fixes compilation warning in aes1610 (965 bytes, patch)
2018-03-11 20:42 UTC, Mark Harfouche
Details | Splinter Review
[Patch] lib: fix compilation warning (2.13 KB, patch)
2018-03-11 20:42 UTC, Mark Harfouche
Details | Splinter Review
[Patch] lib: fixes compilation warning in aes1610 (965 bytes, patch)
2018-03-11 20:42 UTC, Mark Harfouche
Details | Splinter Review
[Patch] lib: Fixes compilation warning in aes2501 (1.21 KB, patch)
2018-03-11 20:42 UTC, Mark Harfouche
Details | Splinter Review
lib: Fixes compilation warning in aes2501 (1.20 KB, patch)
2018-03-11 20:56 UTC, Mark Harfouche
Details | Splinter Review
lib: fixes compilation warning in aes1610 (957 bytes, patch)
2018-03-11 20:56 UTC, Mark Harfouche
Details | Splinter Review
lib: fix compilation warning (2.12 KB, patch)
2018-03-11 20:56 UTC, Mark Harfouche
Details | Splinter Review
aes2501: Fix state machine never using "init_3" state (1.06 KB, patch)
2018-03-11 21:41 UTC, Bastien Nocera
Details | Splinter Review
mindtct: Fix compilation warnings (2.12 KB, patch)
2018-03-11 21:41 UTC, Bastien Nocera
Details | Splinter Review
aes1610: Fix compilation warning in aes1610 (951 bytes, patch)
2018-04-27 10:27 UTC, Bastien Nocera
Details | Splinter Review

Description Mark Harfouche 2018-03-10 18:23:48 UTC
Created attachment 137970 [details] [review]
proposed patch

A few compiler warnings were being sent

```
if(True)
   return a;

   return b;
```
was warning that `return b;` wasn't part o the if statement. 

I added 

```
if(True)
   return a;
else
   return b;
```

Seems like my text editor also stripped whitespaces. If that is a problem, I'll redo the fixes changing the settings on my editor.

Second, a few static variables in drivers were not being used. I added __attribute__((unused)) to them so as to suppress the gcc warning.

I'm not too sure how that affects other compilers.
Comment 1 Bastien Nocera 2018-03-11 12:52:48 UTC
Please split up this patch into 3 independent git-formatted patches, and make sure to remove white space changes from your patch.

Unused static variables should be removed, and the actual gcc warnings should be included in the commit message to make it clear which warning those changes fix.

You can use "git-bz" to easily upload patches to Bugzilla, for example "git bz attach master" will upload the commit referenced as "master" (the last one on the local master branch) to the Bugzilla bug mentioned in the commit message.

See my patches to bug 105427 for examples.
Comment 2 Mark Harfouche 2018-03-11 14:45:20 UTC
Git formatted patches: OK

Whitespace: I generally like the "do not add whitespace" mantra, and would ask people to remove "added whitespace", but I'm sure removing whitespace from the license terms will help many with editors that automatically strip whitespace.

Static variables: I disagree that we should remove these variables. These drivers were likely reverse engineered and the messages left by the initial authors should remain even if they were not found immediately useful. I can comment them out, but I thought that adding an attribute was "cleaner".

git-bz: Is this project officially hosted on github/gitlab? 
Running these manual commands is tedious for the both of us. 

I'm glad to help, but this seems like an unnecessary barrier to entry to learn a bunch of these commands. 

How do you even know my patches will build? 
Do you have a way to compile the updates that I added? 
This patch only adds 1 statement to 4 lines, but others might be more complex and require the help of the compiling in finding typos.
Comment 3 Mark Harfouche 2018-03-11 20:25:35 UTC
Created attachment 137992 [details] [review]
Commented out unused static variables.

As opposed to using __attribute__((unused))

This fixed the following two warnings:
```
drivers/aes1610.c:736:34: warning: ‘stop_reader’ defined but not used [-Wunused-const-variable=]
drivers/aes2501.c:671:34: warning: ‘init_3’ defined but not used [-Wunused-const-variable=]
```

The previous commit fixes -Wmisleading-indentation
```
nbis/mindtct/morph.c:152:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
nbis/mindtct/morph.c:176:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
nbis/mindtct/morph.c:200:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
nbis/mindtct/morph.c:222:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
```
Comment 4 Mark Harfouche 2018-03-11 20:31:06 UTC
Created attachment 137993 [details] [review]
Removes compiler warnings in morph.c
Comment 5 Mark Harfouche 2018-03-11 20:42:06 UTC
Created attachment 137994 [details] [review]
[Patch] lib: Fixes compilation warning in aes2501

Fixes
```
drivers/aes2501.c:671:34: warning: ‘init_3’ defined but not used [-Wunused-const-variable=]
```
Comment 6 Mark Harfouche 2018-03-11 20:42:15 UTC
Created attachment 137995 [details] [review]
[Patch] lib: fixes compilation warning in aes1610

Fixes:
```
drivers/aes1610.c:736:34: warning: ‘stop_reader’ defined but not used [-Wunused-const-variable=]
```
Comment 7 Mark Harfouche 2018-03-11 20:42:23 UTC
Created attachment 137996 [details] [review]
[Patch] lib: fix compilation warning

```
nbis/mindtct/morph.c:152:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
nbis/mindtct/morph.c:176:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
nbis/mindtct/morph.c:200:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
nbis/mindtct/morph.c:222:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
```
Comment 8 Mark Harfouche 2018-03-11 20:42:32 UTC
Created attachment 137997 [details] [review]
[Patch] lib: fixes compilation warning in aes1610

Fixes:
```
drivers/aes1610.c:736:34: warning: ‘stop_reader’ defined but not used [-Wunused-const-variable=]
```
Comment 9 Mark Harfouche 2018-03-11 20:42:37 UTC
Created attachment 137998 [details] [review]
[Patch] lib: Fixes compilation warning in aes2501

Fixes
```
drivers/aes2501.c:671:34: warning: ‘init_3’ defined but not used [-Wunused-const-variable=]
```
Comment 10 Mark Harfouche 2018-03-11 20:45:36 UTC
I think this addresses all the previously mentioned issues
Comment 11 Mark Harfouche 2018-03-11 20:56:05 UTC
Created attachment 137999 [details] [review]
lib: Fixes compilation warning in aes2501

Fixes
```
drivers/aes2501.c:671:34: warning: ‘init_3’ defined but not used [-Wunused-const-variable=]
```
Comment 12 Mark Harfouche 2018-03-11 20:56:12 UTC
Created attachment 138000 [details] [review]
lib: fixes compilation warning in aes1610

Fixes:
```
drivers/aes1610.c:736:34: warning: ‘stop_reader’ defined but not used [-Wunused-const-variable=]
```
Comment 13 Mark Harfouche 2018-03-11 20:56:18 UTC
Created attachment 138001 [details] [review]
lib: fix compilation warning

```
nbis/mindtct/morph.c:152:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
nbis/mindtct/morph.c:176:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
nbis/mindtct/morph.c:200:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
nbis/mindtct/morph.c:222:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
```
Comment 14 Mark Harfouche 2018-03-11 20:57:49 UTC
Second try: 
I think this addresses all the previously mentioned issues.
I think I am also following the naming convention.
Comment 15 Bastien Nocera 2018-03-11 21:41:10 UTC
Created attachment 138002 [details] [review]
aes2501: Fix state machine never using "init_3" state

This fixes this warning by the same token:
drivers/aes2501.c:671:34: warning: ‘init_3’ defined but not used [-Wunused-const-variable=]
Comment 16 Bastien Nocera 2018-03-11 21:41:16 UTC
Created attachment 138003 [details] [review]
mindtct: Fix compilation warnings

nbis/mindtct/morph.c:152:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
nbis/mindtct/morph.c:176:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
nbis/mindtct/morph.c:200:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
nbis/mindtct/morph.c:222:4: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
Comment 17 Bastien Nocera 2018-03-11 21:42:59 UTC
(In reply to Bastien Nocera from comment #15)
> Created attachment 138002 [details] [review] [review]
> aes2501: Fix state machine never using "init_3" state
> 
> This fixes this warning by the same token:
> drivers/aes2501.c:671:34: warning: ‘init_3’ defined but not used
> [-Wunused-const-variable=]

This looked like a copy/paste typo, so I re-did it. Vasily, can you please review?

I'm also unsure about attachment 138000 [details] [review], again Vasily?
Comment 18 Bastien Nocera 2018-03-11 21:43:32 UTC
Comment on attachment 138001 [details] [review]
lib: fix compilation warning

Attachment 138003 [details] pushed as 54deaa1 - mindtct: Fix compilation warnings
Comment 19 Bastien Nocera 2018-03-11 21:46:19 UTC
Thanks for the updated patches Mark.

FWIW, we'll be moving to a hosted gitlab on Freedesktop.org as soon as it's deployed. Many people aren't used to using bugzilla and git together, but a number of us have been doing it through Freedesktop.org and GNOME for a number of years and we're used to it.

I guess my mention of git-bz was helpful, seeing the speed at which the patches were attached ;)

Thanks for your patience in the meanwhile
Comment 20 Mark Harfouche 2018-03-11 22:15:20 UTC
Re git-bz, it was definitely helpful, though still very clunky IMO.


Was there a way to to make git-bz remove whitespace?
Is there a way for git-bz to upload all patches between branches? I had to do it one at a time with:


```
git-bz attach bugs.freedesktop.org:105429 HEAD^^
git-bz attach bugs.freedesktop.org:105429 HEAD^
git-bz attach bugs.freedesktop.org:105429 HEAD
```

I guess the github way of doing things is to do: "Squash + Merge".
Having setup a simple
github+TravisCI+appveyor automated compilations
I am pretty impressed with how much it helps with code review.

I guess copy paste didn't work as well as I hoped, sorry about that.

It also seems like you found the real bug in aes2501.
Comment 21 Bastien Nocera 2018-03-11 22:30:06 UTC
(In reply to Mark Harfouche from comment #20)
> Re git-bz, it was definitely helpful, though still very clunky IMO.
> 
> 
> Was there a way to to make git-bz remove whitespace?

git-bz doesn't touch your commits content, just the commit message to add references if necessary.

> Is there a way for git-bz to upload all patches between branches? I had to
> do it one at a time with:
> 
> 
> ```
> git-bz attach bugs.freedesktop.org:105429 HEAD^^
> git-bz attach bugs.freedesktop.org:105429 HEAD^
> git-bz attach bugs.freedesktop.org:105429 HEAD
> ```

Maybe:
git-bz attach bugs.freedesktop.org:105429 HEAD~2
?

And you don't need to use the bugs.fd.o:XXX reference if you already have a bug url in the commit message (so you could upload to multiple bugs at once).

> I guess the github way of doing things is to do: "Squash + Merge".
> Having setup a simple
> github+TravisCI+appveyor automated compilations
> I am pretty impressed with how much it helps with code review.
> 
> I guess copy paste didn't work as well as I hoped, sorry about that.
> 
> It also seems like you found the real bug in aes2501.

We'll see what Vasily makes of it, I really don't work on the drivers, just the library core and fprintd.
Comment 22 Bastien Nocera 2018-04-27 10:26:47 UTC
Comment on attachment 138003 [details] [review]
mindtct: Fix compilation warnings

Marking as obsolete as it's already been committed.
Comment 23 Bastien Nocera 2018-04-27 10:27:07 UTC
Created attachment 139163 [details] [review]
aes1610: Fix compilation warning in aes1610

Fixes:
drivers/aes1610.c:736:34: warning: ‘stop_reader’ defined but not used [-Wunused-const-variable=]
Comment 24 Bastien Nocera 2018-04-27 10:27:28 UTC
Comment on attachment 138000 [details] [review]
lib: fixes compilation warning in aes1610

Replace by the newest patch
Comment 25 Bastien Nocera 2018-05-25 11:22:57 UTC
Attachment 138002 [details] pushed as bc30a3d - aes2501: Fix state machine never using "init_3" state
Attachment 139163 [details] pushed as 391f77c - aes1610: Fix compilation warning in aes1610


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.