search

iceman1001 / proxmark3

[Deprecated] Iceman Fork, the most totally wicked fork around if you are into proxmark3
http://www.icedev.se/pm3.aspx
GNU General Public License v2.0
465 stars 116 forks source link

Rename prnlog() to PrintfAndLog() #173

Closed brianpow closed 6 years ago

brianpow commented 6 years ago

Rename prnlog() to PrintfAndLog(), just for consistent naming conversion.

iceman1001 commented 6 years ago

This PR introduces name ambiquity between PrintAndLog vs PrintfAndLog. It doesn't solve anything.

brianpow commented 6 years ago

Yes, it does not introduce any new features. But will be a stepping stone to my next pull request.

For ambiquity, I feel prnlog() is even more confusing. I have to look at the source code to understand the differences when I first see it. For the new name PrintfAndLog(), I can almost immediately know the difference from PrintAndLog() from the name

iceman1001 commented 6 years ago

the dual functions before wasn't good at all, however, this PR doesn't change that. What are your next PR?

brianpow commented 6 years ago

PrintAndLog provides slightly better performance while PrintfAndLog provides better flexibility. Just like we have sprint, vsprintf.., having dual functions are difficult to avoid.

I am thinking to add PrintfAndLogError, PrintfAndLogInfo, PrintfAndLogWarning, which automatically add the corresponding prefix (I.e. [+], [-] etc) you introduced recently. IMHO, it will be better than hardcode them in the message.

iceman1001 commented 6 years ago

...well.. prnlog exists when Holiman made a debug printing for code on both device and client. PrintAndLog is client.
dbprintf is device.

However having code like under the folder common which is shared between both device and client, where we don't have printf & logtofile while we don't want a messy code..

So the namechange doesn't solve that issue, where we would have one command for this feature.

However, yes, I like the idea of warning / info message , since that is working with the prefix idea. it will be an easy way for to add the ansi-colors aswell :) which is the direction i want the client to take.

The naming convention is not optimal. What do you think about:

PrintAndLogEx( loglevel, and the rest of input) A couple of defines:

define WARNING(s) PrintAndLogEx( loglevel.warning, #s)

define ERROR(s) PrintAndLogEx(loglevel.error, #s)

define INFO(s) PrintAndLogEx(loglevel.info, #s)

Where we could easily add red, green, blue etc in one place for different messages. The above suggestions are not complete codewise but should give an idea.

brianpow commented 6 years ago

Thanks for the history. However, I see prnlog() just do vsprintf() and call PrintAndLog(), did I miss anything?

iceman1001 commented 6 years ago

If you look at fileutils.c where the prnlog exits, you see the #ifndef ON_DEVICE.. https://github.com/iceman1001/proxmark3/blob/375648ca826f0e37d6a007804efdd780401f8082/client/loclass/fileutils.c#L150

Even more confusing, the prnt .... https://github.com/iceman1001/proxmark3/blob/375648ca826f0e37d6a007804efdd780401f8082/common/protocols.c#L20

https://github.com/iceman1001/proxmark3/blob/375648ca826f0e37d6a007804efdd780401f8082/common/lfdemod.c#L55

So, what we have is two similar constructs, prints but only if its on client, and not on device.

iceman1001 commented 6 years ago

I merge this, and we can have a go at the next ones like prnt, and error/warning. keep up the good work!