| Summary: | Patch: Fixes compiler warnings in lib | ||
|---|---|---|---|
| Product: | libfprint | Reporter: | Mark Harfouche <mark.harfouche> |
| Component: | libfprint | Assignee: | libfprint-bugs |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | medium | CC: | bugzilla, mark.harfouche |
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
proposed patch
Commented out unused static variables. Removes compiler warnings in morph.c [Patch] lib: Fixes compilation warning in aes2501 [Patch] lib: fixes compilation warning in aes1610 [Patch] lib: fix compilation warning [Patch] lib: fixes compilation warning in aes1610 [Patch] lib: Fixes compilation warning in aes2501 lib: Fixes compilation warning in aes2501 lib: fixes compilation warning in aes1610 lib: fix compilation warning aes2501: Fix state machine never using "init_3" state mindtct: Fix compilation warnings aes1610: Fix compilation warning in aes1610 |
||
|
Description
Mark Harfouche
2018-03-10 18:23: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. 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. 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] ``` Created attachment 137993 [details] [review] Removes compiler warnings in morph.c 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=] ``` 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=] ``` 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] ``` 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=] ``` 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=] ``` I think this addresses all the previously mentioned issues 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=] ``` 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=] ``` 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] ``` Second try: I think this addresses all the previously mentioned issues. I think I am also following the naming convention. 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=] 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] (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 on attachment 138001 [details] [review] lib: fix compilation warning Attachment 138003 [details] pushed as 54deaa1 - mindtct: Fix compilation warnings 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 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. (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 on attachment 138003 [details] [review] mindtct: Fix compilation warnings Marking as obsolete as it's already been committed. 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 on attachment 138000 [details] [review] lib: fixes compilation warning in aes1610 Replace by the newest patch 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.