[pisa] Re: Unused functions in pisa
- From: Diego Biurrun <diego@xxxxxxxxxx>
- To: pisa@xxxxxxxxxxxxx
- Date: Thu, 10 Dec 2009 14:59:08 +0100
On Wed, Dec 09, 2009 at 03:18:27PM +0100, Thomas Jansen wrote:
> On Tue, Dec 08, 2009 at 08:22:09PM +0100, Diego Biurrun wrote:
> > On Sun, Nov 29, 2009 at 11:45:26AM +0100, Tobias Heer wrote:
> > > ./libpisa/debug.c:522:void pisa_uint8_to_binstring(uint8_t val, char
> > > *buffer)
> > > ./libpisa/debug.h:150:void pisa_uint8_to_binstring(uint8_t val, char
> > > *buffer);
> > > ./libpisa/debug.c:543:void pisa_uint16_to_binstring(uint16_t val, char
> > > *buffer)
> > > ./libpisa/debug.h:151:void pisa_uint16_to_binstring(uint16_t val, char
> > > *buffer);
> > > ./libpisa/debug.c:564:void pisa_uint32_to_binstring(uint32_t val, char
> > > *buffer)
> > > ./libpisa/debug.h:152:void pisa_uint32_to_binstring(uint32_t val, char
> > > *buffer);
> >
> > I think these can go away, Thomas?
>
> This is rather specialized debug code to get a binary representation of a
> uintXY_t. I'd say it's not needed. If you really need to know what the value
> of a certain bit is, you can easily write some printf("%s", variable&mask ? :
> "true" : "false); code. Or you could print the whole variable and use a
> calculator. Even the hexdump used above could be used.
>
> Verdict: Remove.
Removed, everything else kept.
I also removed a few unused inline functions and moved some that only
got used in one place to the corresponding .c file.
> > > ./libpisa/packet.h:137:uint8_t pisa_get_packet_version(const pisa_packet
> > > *pkt);
> > > ./libpisa/packet.c:131:uint8_t pisa_get_packet_version(const pisa_packet
> > > *pkt)
> > > ./libpisa/packet.h:140:pisa_packet_flags pisa_get_packet_flags(const
> > > pisa_packet *pkt);
> > > ./libpisa/packet.c:164:pisa_packet_flags pisa_get_packet_flags(const
> > > pisa_packet *pkt)
> > > ./libpisa/packet.h:145:void pisa_set_packet_flags(pisa_packet *pkt,
> > > pisa_packet_flags flags);
> > > ./libpisa/packet.c:208:void pisa_set_packet_flags(pisa_packet *pkt,
> > > pisa_packet_flags flags)
> >
> > There are similar functions for other struct members, but the functions
> > are very trivial. Keep? Remove? Thomas?
>
> Obviously, the uint8_t fields of pisa_packet don't need byteorder conversion,
> rendering the accessor functions useless. The reason to keep them might be the
> consistent API for manipulating pisa_packet structures. Also note, that
> pisa_get_packet_version is used in packet.h in the inline function at the
> bottom of the file. So I'd rather keep them.
>
> Verdict: Keep.
>
> However I see another interesting question in that context: What exactly is
> the
> purpose of the flags field in the structure? Right now we don't have any flags
> defined and I don't see that coming in the near future. Might be an option to
> throw out the flags from the structure (and because of the packet format
> change
> bump the version number). Then we could get rid of the two accessor functions
> for flags and keep the get version one, which is a false positive anyway.
What do you mean by false positive?
Otherwise this sounds like a good idea, implement it :)
Diego
Other related posts:
