diff mbox series

[v2,04/13] app/test: fix macro definition

Message ID 20200408031351.4288-5-l.wojciechow@partner.samsung.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series Fixes and unit tests for librte_security |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Lukasz Wojciechowski April 8, 2020, 3:13 a.m. UTC 
  Wrap RTE_TEST_TRACE_FAILURE macro definition into #ifndef clause
as it might be already defined.

Fixes: 5afc521eac6a ("eal: add test assert macros")
Cc: pbhagavatula@caviumnetworks.com

Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 app/test/test.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Thomas Monjalon April 8, 2020, 12:53 p.m. UTC | #1 
08/04/2020 05:13, Lukasz Wojciechowski:
> Wrap RTE_TEST_TRACE_FAILURE macro definition into #ifndef clause
> as it might be already defined.

I think it should not be defined at all.
Why not including rte_test.h?
Lukasz Wojciechowski April 8, 2020, 4:15 p.m. UTC | #2 
Hi Thomas,

Before my patch there was just a definition:
#define RTE_TEST_TRACE_FAILURE TEST_TRACE_FAILURE
without #ifndef condition.

It caused a build problem to me when working on security test, which 
uses both rte_test.h and test.h
As libraries should go first on the include list before local files I used:

#include <rte_test.h>
#include "test.h"

sequence, which cause obvious build error as RTE_TEST_TRACE_FAILURE was 
first defined as an empty macro inside rte_test.h, and redefinition in 
test.h caused a problem.


So I had two ways to solve the issue:
1) to wrap it with #ifndef condition and leave the definition there
2) to remove the redefinition from test.h

I've chosen the 1) solution because:
* Author of the former patch had placed the definition there for some 
purpose
* In my opinion it is better to have the definition present and pointing 
to the same macro for both RTE_TEST_TRACE_FAILURE and TEST_TRACE_FAILURE 
as it would make logs look more consistent when printing information the 
same way.

Best regards
Lukasz


W dniu 08.04.2020 o 14:53, Thomas Monjalon pisze:
> 08/04/2020 05:13, Lukasz Wojciechowski:
>> Wrap RTE_TEST_TRACE_FAILURE macro definition into #ifndef clause
>> as it might be already defined.
> I think it should not be defined at all.
> Why not including rte_test.h?
>
>
>
Thomas Monjalon April 8, 2020, 5:47 p.m. UTC | #3 
08/04/2020 18:15, Lukasz Wojciechowski:
> Hi Thomas,
> 
> Before my patch there was just a definition:
> #define RTE_TEST_TRACE_FAILURE TEST_TRACE_FAILURE
> without #ifndef condition.
> 
> It caused a build problem to me when working on security test, which 
> uses both rte_test.h and test.h
> As libraries should go first on the include list before local files I used:
> 
> #include <rte_test.h>
> #include "test.h"
> 
> sequence, which cause obvious build error as RTE_TEST_TRACE_FAILURE was 
> first defined as an empty macro inside rte_test.h, and redefinition in 
> test.h caused a problem.
> 
> 
> So I had two ways to solve the issue:
> 1) to wrap it with #ifndef condition and leave the definition there
> 2) to remove the redefinition from test.h
> 
> I've chosen the 1) solution because:
> * Author of the former patch had placed the definition there for some 
> purpose

Because rte_test.h is more recent and its addition was not complete enough.
rte_test.h should be included in test.h, and overlaps removed.

> * In my opinion it is better to have the definition present and pointing 
> to the same macro for both RTE_TEST_TRACE_FAILURE and TEST_TRACE_FAILURE 
> as it would make logs look more consistent when printing information the 
> same way.

I think solution 2 is better.


PS: please avoid top-posting


> W dniu 08.04.2020 o 14:53, Thomas Monjalon pisze:
> > 08/04/2020 05:13, Lukasz Wojciechowski:
> >> Wrap RTE_TEST_TRACE_FAILURE macro definition into #ifndef clause
> >> as it might be already defined.
> > I think it should not be defined at all.
> > Why not including rte_test.h?
> >
> >
> >
>
Lukasz Wojciechowski April 9, 2020, 2:10 p.m. UTC | #4 
W dniu 08.04.2020 o 19:47, Thomas Monjalon pisze:
> 08/04/2020 18:15, Lukasz Wojciechowski:
>> Hi Thomas,
>>
>> Before my patch there was just a definition:
>> #define RTE_TEST_TRACE_FAILURE TEST_TRACE_FAILURE
>> without #ifndef condition.
>>
>> It caused a build problem to me when working on security test, which
>> uses both rte_test.h and test.h
>> As libraries should go first on the include list before local files I used:
>>
>> #include <rte_test.h>
>> #include "test.h"
>>
>> sequence, which cause obvious build error as RTE_TEST_TRACE_FAILURE was
>> first defined as an empty macro inside rte_test.h, and redefinition in
>> test.h caused a problem.
>>
>>
>> So I had two ways to solve the issue:
>> 1) to wrap it with #ifndef condition and leave the definition there
>> 2) to remove the redefinition from test.h
>>
>> I've chosen the 1) solution because:
>> * Author of the former patch had placed the definition there for some
>> purpose
> Because rte_test.h is more recent and its addition was not complete enough.
> rte_test.h should be included in test.h, and overlaps removed.
>
>> * In my opinion it is better to have the definition present and pointing
>> to the same macro for both RTE_TEST_TRACE_FAILURE and TEST_TRACE_FAILURE
>> as it would make logs look more consistent when printing information the
>> same way.
> I think solution 2 is better.
Ok I'll change this patch and remove the macro definition at all from 
app/test/test.h
I'll publish it with version 3, but I'll wait a bit more for getting 
more comments on version 2
>
>
> PS: please avoid top-posting
Sorry
>
>
>> W dniu 08.04.2020 o 14:53, Thomas Monjalon pisze:
>>> 08/04/2020 05:13, Lukasz Wojciechowski:
>>>> Wrap RTE_TEST_TRACE_FAILURE macro definition into #ifndef clause
>>>> as it might be already defined.
>>> I think it should not be defined at all.
>>> Why not including rte_test.h?
>>>
>>>
>>>
>
>
>
>
>
diff mbox series

Patch

diff --git a/app/test/test.h b/app/test/test.h
index ac0c50616..8ac581cbc 100644
--- a/app/test/test.h
+++ b/app/test/test.h
@@ -22,7 +22,9 @@ 
 # define TEST_TRACE_FAILURE(_file, _line, _func)
 #endif
 
-#define RTE_TEST_TRACE_FAILURE TEST_TRACE_FAILURE
+#ifndef RTE_TEST_TRACE_FAILURE
+# define RTE_TEST_TRACE_FAILURE TEST_TRACE_FAILURE
+#endif
 
 #include <rte_test.h>