[PATCH i-g-t v14 resend 1/3] lib/igt_facts: Library and unit testing for fact tracking

[Kamil Konieczny]

Hi Peter,
On 2025-01-21 at 09:16:19 +0100, Peter Senna Tschudin wrote:
> 
> Hi Kamil,
> 
> 
> On 19.12.2024 13:40, Kamil Konieczny wrote:
> > Hi Peter,
> > On 2024-12-16 at 16:14:18 +0100, Peter Senna Tschudin wrote:
> > 
> > I have few nits about a code, I checked it with checkpatch.pl
> > and looks like some warnings are left, why not removing them?
> > Here are some found by it:
> > 
> > 0001-lib-igt_facts-Library-and-unit-testing-for-fact-trac.patch:824: CHECK:COMPARISON_TO_NULL: Comparison to NULL could be written "fact"
> > #824: FILE: lib/igt_facts.c:736:
> > +       igt_assert(fact != NULL);
> > 
> > 0001-lib-igt_facts-Library-and-unit-testing-for-fact-trac.patch:825: CHECK:BOOL_COMPARISON: Using comparison to true is error prone
> > #825: FILE: lib/igt_facts.c:737:
> > +       igt_assert(fact->present == true);
> 
> There is a very important detail here. It is error prone only when
> the type is not bool. My code is not error prone.
> 
> 

It was error prone maybe some twenty years ago, I checked
current gcc on Ubuntu and it is ok. Despite that, it is
a code style issue. For boolean var bool_var in C language
we write:

	if (bool_var) { ... some code }

We do _not_ write

	if (bool_var == true) // correct but style issue

Same goes for assertions, they are used for checking that
a condition is _true_, so

	assert(bool_var); // better then assert(bool_var == true);

Regards,
Kamil

> > 
> > 0001-lib-igt_facts-Library-and-unit-testing-for-fact-trac.patch:913: CHECK:LINE_SPACING: Please use a blank line after function/struct/union/enum declarations
> > #913: FILE: lib/igt_facts.h:40:
> > +};
> > +extern struct igt_facts_config igt_facts_config;
> 
> Good catch, thank you.
> 
> > 
> > total: 0 errors, 3 warnings, 23 checks, 858 lines checked
> > 
> > 0001-lib-igt_facts-Library-and-unit-testing-for-fact-trac.patch has style problems, please review.
> > 
> > Especially when there are many it is easy to overlook an issue,
> > one such is comparision with 'true', I searched your code for '== true'
> > and one place needs fixing:
> > 
> > grep -n '== true' lib/igt_facts.c
> > 680:    igt_assert(ret == true);
> > 690:    igt_assert(fact->present == true);
> > 733:    igt_assert(fact->present == true);
> > 737:    igt_assert(fact->present == true);
> > 769:    igt_assert(igt_list_empty(&igt_facts_list_pci_gpu_head) == true);
> > 
> > Last line may be an issue, as sometimes pointer comparision could
> > be implemented in runtime as 'return (src - dest)' so please fix all
> > using template:
> 
> I don't think so as the type is bool. I can't imagine a way to make this go
> wrong. Can you give me an example showing how a bool type can be evaluated
> incorrectly? What am I missing here?
> 
> > 
> > igt_assert(val == true); --> igt_assert(val);
> > 
> > In summary, please fix BOOL_COMPARISON, COMPARISON_TO_NULL,
> > LINE_SPACING, PARENTHESIS_ALIGNMENT.
> > 
> > Please remove extern from header, it is not needed:
> > 
> > extern struct igt_facts_config igt_facts_config;
> 
> yep yep, this is twice on the email...
> 
> [...]