Clean up #defines - Githubissues

arfvidsonUIS commented 8 years ago

Greg's comments

Are these even used?
define MAX_NAME_SIZE 128
name of what? I don't think you even use this in the file!
define MAX_IP_SIZE 50
Huh? You don't use this either?
define MAXOUTSTANDINGCHANNELCOMMAND 256
Here, have a '_', they are free.

But again, I don't see this being used.

arfvidsonUIS commented 8 years ago

Branch has been pushed to githubissue-83 and is ready for review

arfvidsonUIS commented 8 years ago

Just new branch that only modifies the controlvmchannel.h pound defines. Branch name is: githubissue-83-v2. Please review my work. I'll continue working on removing the rest of pound defines so I will be adding additional patches to this branch and will update the github ticket. 💃 :D

davidker commented 8 years ago

Can you squash the last one with the reserved error messages with the first one that removes them? I would like to keep that the same patch so we don't have any time when people don't know they are reserved Also in the fix comments can you get rid of references to SWITCH_ messages ?

davidker commented 8 years ago

Erik, are you still in question mode with this one or is there a v3? Also, I'm still seeing SWITCH_ messages, are you planning on doing that as a separate patch or should we create a new issue for it?

selltc commented 8 years ago

These commits to githubissue-83 branch:

commit b94dc3ad5530cd6eed739b877c436423f80b42e0 staging: unisys: visornic_main.c remove unused pound defines
commit 1c215c1d7a6b469e0184c8d1da505c2f0b87e275 staging: unisys: visorbus_main.c remove unused pound defines
commit 347176dbcafeb35de09b97792595cc0004004819 staging: unisys: visorchipset.c remove unused pound defines

were committed by Greg to staging-next on 9/12 as:

commit fbb5d8cf680b4bc7e8d73b86229055c60b1c6820 staging: unisys: visornic_main.c remove unused pound defines
commit 992d03d91b4cdc141957d2876ab75d83a30d18bc staging: unisys: visorbus_main.c remove unused pound defines
commit f6376a59017847bb107b01f17bf6594c65901c03 staging: unisys: visorchipset.c remove unused pound defines

selltc commented 8 years ago

Note: I am leaving this issue open, as I believe the above commits are only PART of the fix for this issue.

selltc commented 8 years ago

From what I see in the branches githubissue-83-v2 branch and githubissue-83-v3 branch, it appears as if these patches are also part of this issue. Greg accepted these into staging-next on 9/12 as:

commit f6573adedb7d8ee4be3a88fdb2b2e944695946d7 staging: unisys: visorbus: cleanup include listings
commit 4f3f0c3d7c3d0e6532e355068e0f0dc195380356 staging: unisys: visorbus: merge guestlinuxdebug with vmcallinterface.h
commit 7f5780a266cd0fd005242c68f8afc755ebbf9769 staging: unisys: remove check for GNUC
commit dca0a14df16cea02ac690443487995fe4424fae9 staging: unisys: visorbus: Move non global guestlinuxdebug to visorbus
commit 443c0c7ae95de44da1b92f371f23a181a9786076 staging: unisys: visorbus: remove include for vbuschannel.h
commit 77190cfc458af4b04b135f09ae9b0f892449ee40 staging: unisys: visorbus: Merge iovmcall_gnuc into vmcallinterface
commit b6d0fa15e851cc8779a5e7676f2b52a33baba8ba staging: unisys: visorbus: merge vbusdeviceinfo.h into vbuschannel
commit 3995c5da25e988b5699989912a2024a10c0d1b95 staging: unisys: visorbus: Reorder functions to avoid including header
commit 8a7a2fde4d8a7a24e0ca6acc5e032ddedc0b6640 staging: unisys: visorbus: Merge controlvm completion status to controlvmchannel

I'm leaving this issue open because I'm not sure if other patches are needed.

arfvidsonUIS commented 8 years ago

Yeah, I'm just cleaning thing ups will be pushing a new branch soon. I've been working on a Presentation and trac and having gotten around to completing this yet. Please don't close this issue yet. Thanks for the follow up

arfvidsonUIS commented 8 years ago

just pushed the branch to https://github.com/davidker/unisys/commits/githubissue-83-v3. Ready to be inspected

davidker commented 7 years ago

Got the updated branch and the patches are no long complaining about checkpatch warnings. The unused #define count went from 57 to 32. I'll submit these patches, but do you consider the story done yet?

arfvidsonUIS commented 7 years ago

updated branch githubissue-83-v3 and ready for review

arfvidsonUIS commented 7 years ago

check by running ./test_usage.sh 2>/dev/null | cut -d -f 1 | xargs -I '{}' grep -r -n '{}' . I left maxnum on purpose doesn't make sense to have maxmin without a the maxnum.

davidker commented 7 years ago

Erik, what about MAXNUM in iochannel.h? ffee0261160901b1c2a93177c02f9ebecb624fc3

I think that one gets touched as well, I'm also seeing about 11 messages in controlvmchannel.h, am I missing something or are they intended to stay?

kershnda@USTR-ERL-4458[2933.master]:~/kernel/master/unisys/drivers/staging/unisys$ ~/bin/test_usage.sh 2>/dev/null | wc -l 12 kershnda@USTR-ERL-4458[2933.master]:~/kernel/master/unisys/drivers/staging/unisys$ ~/bin/test_usage.sh 2>/dev/null CONTROLVM_RESP_ERROR_CHANNEL_INVALID not used, found in ./visorbus/controlvmchannel.h? CONTROLVM_RESP_ERROR_VIRTPCI_DRIVER_FAILURE not used, found in ./visorbus/controlvmchannel.h? CONTROLVM_RESP_ERROR_VIRTPCI_DRIVER_CALLBACK_ERROR not used, found in ./visorbus/controlvmchannel.h? CONTROLVM_RESP_ERROR_GENERIC_DRIVER_CALLBACK_ERROR not used, found in ./visorbus/controlvmchannel.h? CONTROLVM_RESP_ERROR_BUS_DEVICE_ATTACHED not used, found in ./visorbus/controlvmchannel.h? CONTROLVM_RESP_ERROR_CHANNEL_TYPE_UNKNOWN not used, found in ./visorbus/controlvmchannel.h? CONTROLVM_RESP_ERROR_CHANNEL_SIZE_TOO_SMALL not used, found in ./visorbus/controlvmchannel.h? CONTROLVM_RESP_ERROR_CHIPSET_SHUTDOWN_FAILED not used, found in ./visorbus/controlvmchannel.h? CONTROLVM_RESP_ERROR_CHIPSET_SHUTDOWN_ALREADY_ACTIVE not used, found in ./visorbus/controlvmchannel.h? CONTROLVM_RESP_ERROR_CHIPSET_STOP_FAILED_BUS not used, found in ./visorbus/controlvmchannel.h? CONTROLVM_RESP_ERROR_CHIPSET_STOP_FAILED_SWITCH not used, found in ./visorbus/controlvmchannel.h? MAXNUM not used, found in ./include/iochannel.h?

arfvidsonUIS commented 7 years ago

Update githubissue-83-v3 branch with the additional pound defines that needed to be removed

arfvidsonUIS commented 7 years ago

I also added min_t macro kernel.h so that MINNUM macro could be removed

arfvidsonUIS commented 7 years ago

On another note iochannel.h comments are inconsistent. ie /*

paragraph ... */

when most of them are /*paragraph

continuation of paragraph ... */

Some paragraph start with Uppercase when other do not.

I know I'm nitpicking and is a quick fix. Let me know if you think any of this are necessary to fix and I'll create githubissue. Thing is that if we start fixing this little things we might uncover additional nitpicks I'm not sure if it is worth our time to pursue this.

BTW added feature to your script so we get the linenumber of the unused pound defines ./test_usage.sh 2>/dev/null | cut -d -f 1 | xargs -I '{}' grep -r -n '{}' .

davidker commented 7 years ago

Thanks for the addition to the script, I'll update it as well.

Go ahead and write the KanBoard issues for the nitpicks, but make them lower priority than Greg's emails. I would like to have a full list of all the nits that we know of and can address. I'm not sure what happens with the copyrights on the files since we are in the upstream, I wonder if we should email Jes about that nit.

davidker commented 7 years ago

I've updated the script to include the line number. It can found in the svn repo maintainer_scripts/bin.

davidker commented 7 years ago

Fails to compile, pulling the patches and just sending series up as is.

n file included from ../drivers/staging/unisys/visornic/visornic_main.c:29:0: ../drivers/staging/unisys/include/iochannel.h: In function 'add_physinfo_entries': ../drivers/staging/unisys/include/iochannel.h:536:41: error: macro "min_t" requires 3 arguments, but only 2 given (u16)min_t(len, (u32)PI_PAGE_SIZE); ^ ../drivers/staging/unisys/include/iochannel.h:536:13: error: 'min_t' undeclared (first use in this function) (u16)min_t(len, (u32)PI_PAGE_SIZE); ^ ../drivers/staging/unisys/include/iochannel.h:536:13: note: each undeclared identifier is reported only once for each function it appears in make[5]: * [drivers/staging/unisys/visornic/visornic_main.o] Error 1 make[4]: * [drivers/staging/unisys/visornic] Error 2 make[4]: * Waiting for unfinished jobs.... LD kernel/built-in.o In file included from ../drivers/staging/unisys/visorhba/visorhba_main.c:27:0: ../drivers/staging/unisys/include/iochannel.h: In function 'add_physinfo_entries': ../drivers/staging/unisys/include/iochannel.h:536:41: error: macro "min_t" requires 3 arguments, but only 2 given (u16)min_t(len, (u32)PI_PAGE_SIZE); ^ ../drivers/staging/unisys/include/iochannel.h:536:13: error: 'min_t' undeclared (first use in this function) (u16)min_t(len, (u32)PI_PAGE_SIZE); ^ ../drivers/staging/unisys/include/iochannel.h:536:13: note: each undeclared identifier is reported only once for each function it appears in LD drivers/base/built-in.o make[5]: * [drivers/staging/unisys/visorhba/visorhba_main.o] Error 1 make[4]: * [drivers/staging/unisys/visorhba] Error 2 make[3]: * [drivers/staging/unisys] Error 2 make[2]: * [drivers/staging] Error 2 make[2]: * Waiting for unfinished jobs.... make[1]: * [drivers] Error 2 make[1]: Leaving directory `/home/kershnda/kernel/master/unisys/.build' make: * [sub-make] Error 2 FAILED make

arfvidsonUIS commented 7 years ago

UPDATED: WITH FIX

arfvidsonUIS commented 7 years ago

just remove offset in remove unused pound defines fix comments to 72 look at channel.h and iochannel.h spar firmware pound defines leave everything get rid off

davidker commented 7 years ago

I'm closing this issue. It has been mostly done by Erik's patches and the 2 Code-A-Thons we have done.

davidker / unisys

Clean up #defines #83