Header file cleanup and refactoring

[awakecoding]

Second round of refactoring and cleanup:

Moving libfreerdp-core/constants.h to include/freerdp/constants/_.h Moving include/freerdp/types_.h to include/freerdp/types/.h Moving debug.h to include/freerdp Moving libfreerdp-core/crypto.h to libfreerdp-core/crypto/_.h Merging tls_openssl.c with crypto_openssl.c Merging orderstypes.h with orders.h

[awakecoding]

@otavio: channels/rdpdr/smartcard/scard_operations.c was including it directly, even though it was supposed to be private. I guess scard_operations.c doesn't really need it, so I could remove this include and move debug.h back inside libfreerdp-core?

[awakecoding]

@otavio: just noticed another problem which will come if smartcard debugging is enabled:

ifdef WITH_DEBUG_SCARD

include <libfreerdp/frdp.h>

endif

is there any reason why scard_operations.c is including private stuff like this? should some of it be made public?

[awakecoding]

argh, damn formatting, the last comment showed an # include < libfreerdp / frdp.h > right from scard_operations.c

[awakecoding]

WITH_DEBUG_SCARD is defined in debug.h, so if it goes back inside libfreerdp-core we still have a problem: it's either private or public, but not both at the same time. stuff inside libfreerdp-core should be used by stuff inside libfreerdp-core.

Here is what I suggest: I'd like to add a new macro in debug.h for defining new debug macros. Something like a basic template for new WITH_DEBUG_WHATEVER macros. If you look at the current macros, they're all the same thing copy/pasted over and over again. If we make a simple macro for defining those, and leave debug.h public, it would be easy to define WITH_DEBUG_SCARD in scard_operations.c without having to copy/paste it. This is particularly useful if you want to modify the way the stuff is printed out (for instance, if you want to output to a file instead of using printf) for all debug macros. Instead of having one debug.h header with all debug macros, each debug macro definition will be moved where it actually makes sense. For instance, WITH_DEBUG_NLA being moved to ntlmssp.h.

[otavio]

I agree that scard should be fixed and don't use it as public but in any case debug ought to be private. Even if our libraries inside of freerdp include it, it ought to not be made into include/freerdp since we don't intend it to be used by external clients and like.

[awakecoding]

I know we do not intend to let other programs external to freerdp include it and use it, but how do we avoid copy/pasting inside FreeRDP if it must remain private inside libfreerdp-core?

I'm thinking of moving all the module-specific macros (WITH_DEBUG_NLA, WITH_DEBUG_SCARD, etc) in their respective modules, not to be public. I would still like to give a generic macro in debug.h that can define a new debug class for a module, to avoid copy pasting the same thing over and over again in each module.

[otavio]

I agree with your idea; this makes it clear.

[otavio]

But yes, please the header private.

One possible way is to have include-private dir we use to share stuff between libraries; not pretty but works.

[awakecoding]

@otavio: I'm half-way through the implementation of the change I mentioned, it should help cleaning things up with regards to debug output

[awakecoding]

Just pushed the changes, some additional changes might be required. I got rid of the WITHDEBUG* definitions in debug.h, and made each module declare his own debug macro instead. New debug macros can be declared following a template provided by debug.h. Maybe this could be moved to include/freerdp/utils, where stream.h (stream utils to be used everywhere in FreeRDP) already is.

As you said, including a private dir is not pretty but it works. "not pretty" is not what I'm looking for when my goal is to make the code prettier. Moving debug.h to include/freerdp/utils might be a compromise: no matter what, we do need to share stuff between different FreeRDP components. I'm open to suggestions other than including a private dir though, if anything comes to your mind.

[awakecoding]

I just moved debug.h to include/freerdp/utils/debug.h, I think it makes more sense to see it as a header file containing utility macros now.

[otavio]

While you're on this, please fix the debug macro to put \n automatically at the end of fmt so we avoid adding it in every debug line byhand.

[awakecoding]

@otavio: I agree, but that one is going to modify just about every file :P

[otavio]

It can be done in a separated commit; but makes it clearer ... seems worth it

[awakecoding]

I'll do it today

[awakecoding]

Ok, I have modified the macro to automatically insert a newline character, and I have refactored all the debug code accordingly

[otavio]

You have done a lot of whitespace and comment refactoring on this commit, please split those. Plus please use a proper commit message that fits 80 colums and has a full description, since it can be used later for shortlog and changelog.

[otavio]

The last thing I would see as something sub optimal is the log method; we could refactor it as well to avoid having so many parens and like...

[awakecoding]

@otavio: sorry about that, you're right

Next: libgdi refactoring to resolve naming conflict with real GDI API

[awakecoding]

@otavio: haven't used the log method a lot myself, what do you suggest?

[otavio]

We could use something similar as printk of kernel. They also handle levels of printing without requiring to have two params to the method.

[awakecoding]

I removed the .settings folder, and fixed compilation on Windows. Should we merge now and start a third round of refactoring in another pull request? The next steps are much less related to this.

[otavio]

Seems fine to me.

[otavio]

But please check the commit messages