The logic of searching for consecutive replacements is broken

[oleg-derevenetz]

The current logic here:

clang-tidy-pr-comments/run_action.py

Lines 218 to 233 in 3fae50b

	replacements_are_consecutive = True
	for i in range(len(diagnostic["DiagnosticMessage"]["Replacements"]) - 1):
	current_offset = diagnostic["DiagnosticMessage"]["Replacements"][i][
	"Offset"
	]
	current_length = diagnostic["DiagnosticMessage"]["Replacements"][i][
	"Length"
	]
	next_offset = diagnostic["DiagnosticMessage"]["Replacements"][i + 1][
	"Offset"
	]
	if current_offset + current_length < next_offset - 1:
	replacements_are_consecutive = False
	break
	if replacements_are_consecutive:

is problematic. Consider the following fixes.yml:

Diagnostics:
- BuildDirectory: /home/runner/work/fheroes2/fheroes2/build/src/fheroes2
  DiagnosticMessage:
    FileOffset: 7837
    FilePath: /home/runner/work/fheroes2/fheroes2/src/fheroes2/kingdom/kingdom.cpp
    Message: return type 'const EventsDate' (aka 'const list<EventDate>') is 'const'-qualified
      at the top level, which may reduce code readability without improving const
      correctness
    Ranges:
    - FileOffset: 7837
      FilePath: /home/runner/work/fheroes2/fheroes2/src/fheroes2/kingdom/kingdom.cpp
      Length: 5
    Replacements:
    - FilePath: /home/runner/work/fheroes2/fheroes2/src/fheroes2/kingdom/kingdom.cpp
      Length: 6
      Offset: 7837
      ReplacementText: ''
    - FilePath: /home/runner/work/fheroes2/fheroes2/src/fheroes2/kingdom/kingdom.h
      Length: 6
      Offset: 4838
      ReplacementText: ''
  DiagnosticName: readability-const-return-type
  Level: Warning
MainSourceFile: ''

First of all, these replacements are for different files with different FilePaths in each replacement, but this is not taken into account when checking for continuity. Second, even though these replacements are not consecutive, they are still considered as such, because 7837 + 6 is not less than 4838 - 1.

[platisd]

😫

Is there any entry for DiagnosticMesage where FilePath is kingdom.h too or is this all?

[oleg-derevenetz]

@platisd this is the entire fixes.yml as is :)

[platisd]

So, from what I understand, to reproduce this I need to return a const object and have the declaration and definition in separate files?

I.e.:

// Foo.h
struct Foo {
    const int bar();
};

// Foo.cpp
#include "Foo.h"

const int Foo::bar() {
    return 42;
}

[oleg-derevenetz]

@platisd and also we need to have an offset of the second entry to be no more than offset+length of the first entry, because that's how intersection is currently checked:

     if current_offset + current_length < next_offset - 1: 
         replacements_are_consecutive = False 
         break 

BTW this logic can recognize even fragments of the same file as "consecutive" even if they are in fact not, but they are just specified in "reverse order" for some reason.