meekrosoft / fff

A testing micro framework for creating function test doubles
Other
761 stars 167 forks source link

Feat/weak attribute #47

Closed susundberg closed 5 years ago

susundberg commented 6 years ago

Hi!

Here is pull request for issue #43. I have tested it only with gcc (i guess clang will be tested with the CI) - and used in my own project.

louiske0911 commented 6 years ago

Could I ask you why does need the attribute((weak)) symbol ?

susundberg commented 6 years ago

Hi! I tried to reason the need in issue 43 .

In short, i have a set of files (generated), that define fake functions for all real functions. This is compiled to be library -- say "libfake.a". Now when linking a test binary, i have maybe one or two real functions, and this binary is linked against the fakelib (i have many of those binaries - one for each set of real functions!). Now without the weak one gets linker error -- the real function is defined also in fakelib. With weak linker is instructed to select the other one with no error.

I could surely pick the files for compiling, but that would require additional "makefile"-logic -- now i can compile the whole fakelib once, and let the linker select suitable implementation for required functions.

meekrosoft commented 6 years ago

I am a bit reluctant to merge this one... I completely see the need - but not every compiler supports weak attributes using this syntax so it would break some users. Is there another way? Perhaps a preprocessor switch? I imagining adding a helper macro to output something like this:


#IFDEF GCC_WEAK_FUNCTIONS
   #DEFINE WEAK_OPTION __attribute__((weak))
#ELSE
    #DEFINE WEAK_OPTION 
#
"#{return_type} WEAK_OPTION FUNCNAME(#{arg_val_list(arg_count)}#{varargs})"
meekrosoft commented 6 years ago

Then users could include fff.h to turn on the option:

#DEFINE FFF_GCC_WEAK_FUNCTIONS
#include "fff.h"
wulfgarpro commented 5 years ago

@susundberg, please resolve your conflicts before review moves forward.

AppVeyorBot commented 5 years ago

:x: Build fff 23-appveyor failed (commit https://github.com/meekrosoft/fff/commit/a90dd84fe2 by @susundberg)

AppVeyorBot commented 5 years ago

:white_check_mark: Build fff 24-appveyor completed (commit https://github.com/meekrosoft/fff/commit/1de1f05c54 by @susundberg)

susundberg commented 5 years ago

Hi! Sorry for long delay.

I (force) pushed new update; i replaced the hardcoded __attribute__(weak) with #ifdef FFF_FUNCTION_ATTRIBUTE as i though it would fit better.

I updated also the generated fff.h but there seems to be whitespace differences also on unmodified master. I am running ubuntu 18.04 with

ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-linux-gnu]

Any suggestions what version to use (and maybe add that to readme?).

wulfgarpro commented 5 years ago

@susundberg, the only whitespace changes I can see in your PR are good ones. It looks like you've used your editor to strip them.

susundberg commented 5 years ago

Hi! Sorry for the delay again. I actually did checked out the tests before making the pull request but decided not to go there since it was not clear me how they are structured etc.

I now went back to look how it goes - and would an examples/weak_linking/ with some tests included be sufficient?

Happy holidays!

wulfgarpro commented 5 years ago

Just define an example weak function and fake it, as was the need for this functionality.

On 21 Dec. 2018 1:01 am, "Pauli Salmenrinne" notifications@github.com wrote:

Hi! Sorry for the delay again. I actually did checked out the tests before making the pull request but decided not to go there since it was not clear me how they are structured etc.

I now went back to look how it goes - and would an examples/weak_linking/ with some tests included be sufficient?

Happy holidays!

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/meekrosoft/fff/pull/47#issuecomment-449008466, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6TtLl_9eB5jkvCxAuuW-DBaX14NIPNks5u65gtgaJpZM4TBg_E .

AppVeyorBot commented 5 years ago

:white_check_mark: Build fff 41-appveyor completed (commit https://github.com/meekrosoft/fff/commit/88b177b41e by @susundberg)

susundberg commented 5 years ago

Uh sorry, ignore, force pushed after pulling to latest, examples still missing ..

AppVeyorBot commented 5 years ago

:white_check_mark: Build fff 42-appveyor completed (commit https://github.com/meekrosoft/fff/commit/20af33657d by @)

AppVeyorBot commented 5 years ago

:white_check_mark: Build fff 43-appveyor completed (commit https://github.com/meekrosoft/fff/commit/767dbfaa3f by @)

AppVeyorBot commented 5 years ago

:white_check_mark: Build fff 44-appveyor completed (commit https://github.com/meekrosoft/fff/commit/7818963220 by @)

AppVeyorBot commented 5 years ago

:white_check_mark: Build fff 45-appveyor completed (commit https://github.com/meekrosoft/fff/commit/cdb6a8c5c2 by @)

susundberg commented 5 years ago

Hi all again. I took a while but i added an example "weak_linking". Its compiled and runned on the normal command. The example itself is silly - as you can guess - but the Makefile includes three example cases where weak linking is required.

I did not make any (automatic) check that the compiling fails if the weak linking flag is not set. I did try manually though.

How does it look?

wulfgarpro commented 5 years ago

@susundberg, see merge 0b9e9f5.

Thank you for the contribution.