Re: [PATCH v5 03/10] dtc: dt-check-style: Fix alignment of values in continued property lines

[Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>]

On 26/08/2026 12:34, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] String index calculated from stripped string is used to slice the unstripped string, causing incorrect column alignment if a comment precedes the first '<' or '"'. The `check_continuation_alignment` function uses `_strip_strings_and_comments` to strip comments from `rest`, reducing its length. It then runs `re.search` on `rest` to find `<` or `"`. The resulting match index (`m.start(1)`) is then incorrectly used to slice the original unstripped `dl.raw` string. If a block comment exists between `=` and the first value, the slice truncates prematurely, discarding the visual width of the comment and resulting in a `target_col` that is too small.
> - [Medium] Incomplete detection of closed values for space-separated arrays and concatenated strings causes false positive alignment errors. The `check_continuation_alignment` function tracks whether a value block is closed using `endswith('",') or endswith('>,')`. This strict check assumes that completed values must end with a comma. In valid DTS, cell arrays or strings can be concatenated across lines without commas (e.g., `ranges = <...>
> <...>;` or `prop = "first"
> "second";`). If a line ends with `>` or `"` without a comma, the block is closed, but `dl_value_complete` incorrectly evaluates to `False`. The script then unconditionally applies `target_offset = 1` to the subsequent line, improperly demanding that the new `<...>` or string be indented one space to the right (aligned with inner values) rather than aligning with the leading `<` or `"`.
> --

All rejected.

> 
> commit 3c5fcc59a610ff8a5dc6318a90c848ba441566f3
> Author: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> 
> dtc: dt-check-style: Fix alignment of values in continued property lines
> 
> Continued lines in property assignments should be indented to opening
> '<' or '"' if they also start with that character. If the continued line
> is part of previous phandle, then alignment should be to inner values to
> make it more readable.
> 
>> diff --git a/scripts/dtc/dt-check-style b/scripts/dtc/dt-check-style
>> --- a/scripts/dtc/dt-check-style
>> +++ b/scripts/dtc/dt-check-style
>> @@ -938,17 +939,26 @@ def check_continuation_alignment(ctx):
>>          eq = dl.raw.find('=')
>>          if eq < 0:
>>              continue
>> -        # First '<' or '"' after '='
>> -        rest = dl.raw[eq + 1:]
>> -        m = re.search(r'[<"]', rest)
>> +        # First '<' or '"' after '=', but ignore comments and strip trailing
>> +        # whitespace (e.g. remaining after removing the comment)
>> +        rest = _strip_strings_and_comments(dl.raw[eq + 1:]).rstrip()
>> +        m = re.search(r'\s*([<"])', rest)
>>          if not m:
>>              continue
>> -        target_col = _display_col(dl.raw[:eq + 1 + m.start()])
>> +        dl_value_complete = rest.endswith('",') or rest.endswith('>,')
> 
> [Severity: Medium]
> Does this strict check correctly identify closed values in space-separated
> arrays and concatenated strings?
> 
> In valid DTS, cell arrays or strings can be concatenated across lines without
> commas:
> 
>     ranges = <...>
>              <...>;

Not a valid DTS.

> 
> If a line ends with a closing bracket or double quote without a comma, the
> block is closed, but dl_value_complete will incorrectly evaluate to False here.
> 
>> +        target_col = _display_col(_strip_strings_and_comments(dl.raw[:eq + 1 + m.start(1)]))
> 
> [Severity: Medium]
> Will this calculate the correct column alignment if a block comment exists
> between the = and the first opening bracket or double quote?

Might not, but that's not the code we want.

> 
> Because rest is populated using _strip_strings_and_comments(), any block
> comments are removed, shortening the string. Using m.start(1) from this
> stripped string as an index to slice the unstripped dl.raw discards the
> visual width of those block comments, resulting in a target_col that is
> too small.
> 
>>          for cont in dl.continuations:
>> -            if _display_col(cont.indent_str) != target_col:
>> +            target_offset = 0
>> +            err_msg_explanation = 'to < or "'
>> +            if not dl_value_complete:
>> +                target_offset = 1
>> +                err_msg_explanation = 'to the value under <'
> 
> [Severity: Medium]
> Since dl_value_complete evaluates to False for comma-less concatenations as
> noted above, doesn't this unconditionally apply target_offset = 1?

Comma-less concatenations are not accepted.

> 
> This would force developers to misalign their properly styled code by adding
> an extra space, producing a false positive warning in the style checker.
> 
Best regards,
Krzysztof