erikdoe / ocmock

Mock objects for Objective-C
http://ocmock.org
Apache License 2.0
2.16k stars 606 forks source link

Suppress a warning in OCMockObjectMacroTests #425

Closed matrush closed 4 years ago

matrush commented 4 years ago

This test cannot be ran with these flags on: -Werror,-Wunused-value. The full error looks like this:

stderr: OCMock/src/Source/OCMockTests/OCMockObjectMacroTests.m:541:19: error: expression result unused [-Werror,-Wunused-value]
        OCMVerify([mock init]);
                  ^~~~~~~~~~~
OCMock/src/Source/OCMock/OCMock.h:152:93: note: expanded from macro 'OCMVerify'
#define OCMVerify(...) _OCMVerify_X(,##__VA_ARGS__, _OCMVerify_2(__VA_ARGS__), _OCMVerify_1(__VA_ARGS__))
                                                                                            ^~~~~~~~~~~
OCMock/src/Source/OCMock/OCMock.h:149:52: note: expanded from macro '_OCMVerify_1'
#define _OCMVerify_1(A)                 _OCMVerify(A)
                                                   ^
OCMock/src/Source/OCMock/OCMock.h:122:13: note: expanded from macro '_OCMVerify'
            invocation; \
            ^~~~~~~~~~
OCMock/src/Source/OCMock/OCMock.h:161:5: note: expanded from macro '_OCMSilenceWarnings'
    macro \
    ^~~~~
OCMock/src/Source/OCMock/OCMock.h:151:41: note: expanded from macro '_OCMVerify_X'
#define _OCMVerify_X(x,A,B,FUNC, ...)   FUNC

Basically the issue is that the result of init is not used. So adding some random method call should suppress the warning and fix the build.

dmaclach commented 4 years ago

I think this should already be fixed with #422 ?

dmaclach commented 4 years ago

FWIW this would cover #390

matrush commented 4 years ago

Ah, I didn't see that. Yeah, either one would work. Let's see which @erikdoe will pick :)

erikdoe commented 4 years ago

As I mentioned on #422 I'd rather not put workarounds into OCMock for this. We're talking about a warning that is issued when building OCMock, right? And a warning that is the result of an issue in the toolchain, which we assume will get fixed, right?

matrush commented 4 years ago

Yeah, however, with some existing toolchains, the warning is treated as error sometimes. So that becomes a little bit annoying since it's not welcomed to disable the -Werror,-Wunused-value either.

I think to workaround this for now seems to be slightly better :) As we didn't change any specific functionality/behavior here.

erikdoe commented 4 years ago

I'm not sure I understand. Which toolchains do you use to build OCMock?

matrush commented 4 years ago

Sorry, the problem is not specific to any toolchains. I mean for existing projects, the library might be integrated in different fashions. Sometimes the dev env forces these flag to be set up, so if the warning is not addressed, the whole project will fail to build.

An alternative solution would be to suppress the warning with some macros. But I think the current changes I did doesn't change any meaning of the test itself. So perhaps it's fine to merge it to avoid the warnings? As the purpose of this specific test is not to surface any issues for clang, but to test the right logic itself.

erikdoe commented 4 years ago

So, to clarify: this is for setups that build OCMock from source, likely as part of the build of another product, AND that force these flags to be set. Can I ask how you build OCMock from source and why the flags have to be set for OCMock? I get you might want to set some flags for the code you write, but for external dependencies...?

And, of course, the test is not there to test clang. None of them are. I'm wondering whether to change the test, though, just because clang has an issue at the moment.

matrush commented 4 years ago

Our project is using BUCK and we has some base wrappers for apple_library, and most of the libraries will gets a list of flags inherited from the base rule. Most of them are generally recommended to be turned on, so that we can catch some issues early on. Even for external dependencies, sometimes if we detected these warnings, we can also contribute back to upstreams :) So setting these flags I think is fine.

Again, I think the root problem is not about how we build it from source. These flags are pretty common to be used. I saw we have already have taken action to file a report for clang. So the warning has already served its purpose and can be turned off now with the workaround. I think if we have a way to stop the warning. It's better to suppress it for now. Even clang fixed the issue later (who knows when), as the workaround didn't introduce any maintenance overhead, I think we don't need to take any further action in the future.

erikdoe commented 4 years ago

Fixed in 9b234bd131ccc264cce1bec447b502da7ac843c5.

matrush commented 4 years ago

Thanks @erikdoe!