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

[Peter Senna Tschudin <peter.senna@linux.intel.com>]

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.


> 
> 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...

[...]