Neuer FixInsight ist da!

[Der schöne Günther]

Thank you for your reply.

Regarding excluding code from being checked, you mean like this?
Take this:

Delphi-Quellcode:

unit SomeUnit;

interface

type
   TParent = class
      procedure someVirtualMethod(); virtual;
   end;

   TChild = class(TParent)
      procedure someVirtualMethod(); override;
   end;

implementation

{ TParent }

procedure TParent.someVirtualMethod();
begin
   WriteLn('Quack!');
end;

{ TChild }

{$IFNDEF _FIXINSIGHT_}
procedure TChild.someVirtualMethod();
begin
   // No quack at all
end;
{$ENDIF _FIXINSIGHT_}

end.

Do you think it would also be possible to further specify which warning or convention to ignore? In this case, I'd only like to ignore a W519 but lose all the other cool warnings and conventions as well.

[Stevie]

Zitat:

Zitat von Der schöne Günther (Beitrag 1294880)

Do you think it would also be possible to further specify which warning or convention to ignore? In this case, I'd only like to ignore a W519 but lose all the other cool warnings and conventions as well.

Given how DelphiAST works I think that is not so trivial. The trick with the $IFNDEF just works because it ignores that part of code because the define is set by FixInsight.
For using the same technique it would require multiple parses for each rule or including ifdefs into the AST which currently is not the case.

A different approach could be like rule suppressions in StyleCop work.
But using attributes for such purpose will lead to W1025 when compiling that code unless you add a unit with dummy attributes.

Edit: Ok, maybe a combination of both could work... something like this:

Delphi-Quellcode:

{$IFDEF _FIXINSIGHT_}[SuppressRule(W519)]{$ENDIF}
procedure TChild.someVirtualMethod();
begin
   // No quack at all
end;

[Uwe Raabe]

Zitat:

Zitat von Stevie (Beitrag 1294903)

Delphi-Quellcode:

{$IFDEF _FIXINSIGHT_}[SuppressRule(W519)]{$ENDIF}
procedure TChild.someVirtualMethod();
begin
   // No quack at all
end;

How long is this "SuppressRule" valid: Only for the following method or for the rest of the unit?

We have a similar problem when dealing with string constants and localization tools automagically extracting those from the sources. It is quite common for these tools to accept a special formatted comment to ignore such a constant. If you scan the Delphi source you will find lots of comments like

Delphi-Quellcode:

{ Do not localize }

which are valid for the current line. Perhaps we can have a similar approach for FixInsight warnings.

Delphi-Quellcode:

procedure TChild.someVirtualMethod(); { FIXINSIGHT: SuppressRule(W519) }
begin
   // No quack at all
end;

As the comment lies inside the method implementation the scope should be obvious. Depending on the nature of any rule other intrinsic scopes may be appropriate.

[Stevie]

Zitat:

Zitat von Uwe Raabe (Beitrag 1294916)

How long is this "SuppressRule" valid: Only for the following method or for the rest of the unit?

For exactly the element it was put on. In this case for the entire method. Personally I would not make it too fine grained.
In the end we want to find bad code and not put suppress rule directives all over the place, right?

Zitat:

Zitat von Uwe Raabe (Beitrag 1294916)

We have a similar problem when dealing with string constants and localization tools automagically extracting those from the sources. It is quite common for these tools to accept a special formatted comment to ignore such a constant. If you scan the Delphi source you will find lots of comments like

Delphi-Quellcode:

{ Do not localize }

which are valid for the current line. Perhaps we can have a similar approach for FixInsight warnings.

...

As the comment lies inside the method implementation the scope should be obvious. Depending on the nature of any rule other intrinsic scopes may be appropriate.

As I said, the current implementation of DelphiAST limits the possibilities. Comments won't work either since they are not part of the generated AST (afaik).
To my knowledge FI works on the generated AST. Anything that is not part of that AST cannot be taken into consideration.

[Roman Yankovsky]

Zitat:

Zitat von Der schöne Günther (Beitrag 1294880)

Regarding excluding code from being checked, you mean like this?
Take this:

Yes, exactly like this.

Regarding your second question about ignoring only a specific warning Stefan is right, it's not a trivial task. I'm thinking about implementing this though.

Zitat:

Zitat von Stevie (Beitrag 1294903)

Zitat:

Zitat von Der schöne Günther (Beitrag 1294880)

Do you think it would also be possible to further specify which warning or convention to ignore? In this case, I'd only like to ignore a W519 but lose all the other cool warnings and conventions as well.

Given how DelphiAST works I think that is not so trivial. The trick with the $IFNDEF just works because it ignores that part of code because the define is set by FixInsight.
For using the same technique it would require multiple parses for each rule or including ifdefs into the AST which currently is not the case.

I was thinking about something like this:

Delphi-Quellcode:

{$FIOFF W509}
procedure Method;
begin

end;
{$FION W509}

It's still not trivial, but I think I can handle this directives in DelphiAST by adding a certan attribute to all syntax nodes between $FIOFF and $FION. Not in DelphiAST itself of course, but it is possible to inherit it and override a few methods. And as you have mentioned above, attributes is another option.

[Der schöne Günther]

I don't know about this DelphiAST, but what about the other way around? Just parse as you always do. We now have a list of warnings and the line number it occurred on.

So in case the code goes like this

Delphi-Quellcode:

25:   procedure TChild.someVirtualMethod();
26:   begin
27:       // No quack at all
28:   end;

, there is a W519 raised for line 25. Now before outputting our results to the IDE's listbox, let's check line 25 whether it contains a comment with an exclamation mark followed by "W519".

So if the code is like

Delphi-Quellcode:

25:   procedure TChild.someVirtualMethod(); //!W519 because FixInsight is smart
26:   begin
27:       // No quack at all
28:   end;

, there will be no "false positive".

[Roman Yankovsky]

Nice idea :)

[jaenicke]

This could even be expanded to the whole unit scope (for example for 3rd party units, where only the first line would have to be modified then):

Delphi-Quellcode:

unit Example; //!W519 (cannot be fixed because of...)
...

[freimatz]

Zitat:

Zitat von Roman Yankovsky (Beitrag 1294694)

Zitat:

Zitat von freimatz (Beitrag 1294603)

Hallo,
habe heute mal die Demo installiert. Bei Aufruf mit einem meiner Projekte kommt ein EStringListError in Classes.TFileIterator.Next. (nach madExcept) Eine Ausgabe kommt dann bei continue trotzdem. Umständlich ist, dass mich die meisten units des Projektes gar nicht insteressieren.
Bei Aufruf von Kommandozeile geht gar nichts. D.h. FixInsightCL nacht eine kleine Pause und beendet dann. Zumindest bei --output und --xml hätte ich eine leere xml erwartet.

The EStringListError is fixed today.
Please download the latest build http://sourceoddity.com/download/Fix...upd1_setup.exe

Unfortunately, I cannot reproduce the issue ...

Nach Installation der neuen Version ist alles gut :-D

[schöni]

Ab welcher Delphi Version kann man das einsetzen?

Ab welcher Windows Version funktioniert das Tool?

@freimatz: Ist das gefixte Tool im Link die Demo- oder die Vollversion?

[Roman Yankovsky]

It supports all Delphi versions from Delphi 2006 to XE7.

[Roman Yankovsky]

... to XE8 :)

[Der schöne Günther]

Please have a look at
http://www.delphipraxis.net/1303391-post1.html

Is it possible for FixInsight to raise a W1037? Dcc32 and Dcc64 fail to do so.

[Roman Yankovsky]

Nice idea, thanks.
I will think about implementing this.

[Der schöne Günther]

Just noticed: You raise a W523 "Interface declared without a GUID" for generic interfaces. Generic interfaces cannot have a GUID.

And by the way: I believe a W504 (Missing INHERITED call in constructor/destructor) is not appropriate for a record constructor since they don't support inheritance in the first place.

[Stevie]

Zitat:

Zitat von Der schöne Günther (Beitrag 1307429)

Generic interfaces cannot have a GUID.

They can - their use is just limited because all instantiations have the same GUID - which can be dangerous but also useful when you know what you are doing.
However Roman might want to disable triggering that warning for generic interfaces.

[Whookie]

Nice idea, gave it a short try and here are my first expiriences:

W522 does enforce "override" to be written in lower case (which is not required)
W521 gives an error with:

Delphi-Quellcode:

function test: boolean;
var
  LResult: Boolean;
begin
  try
    LResult := TRUE;
  finally
    Result := LResult;
  end;
end;

W504 does warn on missing inherited in a constructor directly derived from TObject (even though it might be good practice(?)) I never call them in that circumstance.

[Der schöne Günther]

Zitat:

Zitat von Whookie (Beitrag 1307457)

even though it might be good practice(?) I never call them in that circumstance.

I always explicitly derive from

Delphi-Quellcode:

TObject

and call the (empty) inherited constructor. Java, for example, is different: It implicitly calls the inherited parameterless constructor (if there's any). Otherwise, it's a compile time error.

Another reason is that it often happened to me that I omitted the (then redundant) call to the super constructor but later decided to derive the class from something other than

Delphi-Quellcode:

TObject

. Then, it's easy to miss adding the call to the inherited constructor back in. (FixInsight to the rescue! 8-))

[Whookie]

Zitat:

Zitat von Der schöne Günther (Beitrag 1307459)

...but later decided to derive the class from something other than

Delphi-Quellcode:

TObject

. Then, it's easy to miss adding the call to the inherited constructor back in. (FixInsight to the rescue! 8-))

That's one hell of an argument...

[Der schöne Günther]

Zitat:

Zitat von Der schöne Günther (Beitrag 1307429)

And by the way: I believe a W504 (Missing INHERITED call in constructor/destructor) is not appropriate for a record constructor since they don't support inheritance in the first place.

Also, whether class helpers should be checked for a missing "inherited" call in the constructor is also debatable. An "inherited" statement in a class helper constructor is illegal.

[TiGü]

Seit vier Tagen gibt's die Version FixInsight 2015.08
http://sourceoddity.com/fixinsight/download.html

[Roman Yankovsky]

Finally FixInsight 2015.10 is able to suppress a warning for a piece of code (that was discussed on a previous page). It works not for a scope, but for a particular line though.

Example:

Delphi-Quellcode:

      procedure RestartTimer;
      begin
        FTimer.Enabled := False;
        FTimer.Enabled := True; //FI:W508 - this warning will be ignored.
      end;

So it's just a "FI:<RULENUMBER>" comment.

[Der schöne Günther]

Neat! That is WAY better than using IFDEF blocks :thumb:

[Roman Yankovsky]

I would like to announce Christmas bundle. FixInsight and Parnassus Navigator go together.

http://sourceoddity.com/blog/2015/12...sus-navigator/

[Der schöne Günther]

I have updated from 2015.10 to 2015.11 and would like to report a few things:

Fatal Parse error
caused by the following code:

Delphi-Quellcode:

function TSomeClass.calcSomething(const someCount: Integer): Integer;
begin
   Result := 'REG11111;2;33333333;44444444;55555555'.Length * someCount; // <-- This line
   [...]
end;

This did not cause a fatal error in 2015.10

C110: Getter or setter name is different from property declaration
The following code causes a C110. I guess it's because of the "&":

Delphi-Quellcode:

      public property
         &Register[index: TRegisterIndex]: TRegisterValue
            read getRegister
            write setRegister;
            default;

W521 Return value of function might be undefined
The following code did not cause this warning in 2015.10:

Delphi-Quellcode:

type TSomeEnum = (uno, dos, tres);

function TSomeClass.getSomeValue(): TSomeEnum;
begin
   if someCondition() then
      Exit(TSomeEnum.uno)
   else
      if someOtherCondition() then
         Exit(TSomeEnum.dos)
      else
         Exit(TSomeEnum.tres);
end;

O802 ResourceString is declared but never used
That's an excellent idea! :thumb:

W528: Variable not used in for-loop
This is strange one. The following code produces a W528:

Delphi-Quellcode:

unit Unit1;

interface uses System.Generics.Collections, System.SysUtils;

type

   TPair = System.Generics.Collections.TPair<Single, Single>;
   TPairs = TArray<TPair>;

   TPairsExporter = class
      public class function DataAsString(const pairs: TPairs): String;
   end;

implementation uses System.Classes;

class function TPairsExporter.DataAsString(const pairs: TPairs): String;
var
   stringBuilder:   TStringBuilder;
   pair:          TPair;
begin
   stringBuilder := TStringBuilder.Create();
   try
      for pair in pairs do
         stringBuilder.Append(pair.Key).Append(sLineBreak);

      Result := stringBuilder.ToString();
   finally
      stringBuilder.Destroy();
   end;
end;

end.

.

If you remove the ".Append(sLineBreak)", the warning is gone. Oh, and please ignore the fact that using a for..in with records was not the best idea :oops:

[Stevie]

Trag die Sachen doch einfach in den offiziellen bugtracker ein.

[Der schöne Günther]

Consider the following:

Delphi-Quellcode:

   IFirstInterface = interface
      procedure doStuff();
   end;

   ISecondInterface = interface
      procedure doStuff();
   end;

   TImplementation = class(TInterfacedObject, IFirstInterface, ISecondInterface)
      procedure doStuff();
   end;

Compiles and works fine (unlike other languages). But probably not what you want. I know about method resolution clauses and how to get around it. But I would like to get a warning that this does not look right at all.

Opinions?

[jaenicke]

Was findest du daran denn falsch? Auf diese Weise stellen wir z.B. unterschiedliche Interfaces für Skripte und für DLLs oder interne Zwecke zur Verfügung, die durch die gleiche Klasse implementiert werden. Und auch in verschiedenen Versionen eines Interfaces kann es ja die gleichen Methoden geben. Es ist ja nicht immer alles neu.

Auch in der Windows API findest du z.B. mehrere Versionen mancher Interfaces, die so logischerweise auch gleiche Methoden enthalten können.

Das ist ja auch genau der Sinn von Interfaces. Welche Klasse diese implementiert ist egal, und damit auch was die Klasse sonst noch macht.

[Uwe Raabe]

I agree with Sebastian: This is a perfect example of one of those many advantages that Delphi has compared to other languages. I for myself use this technique quite often.

[Roman Yankovsky]

Der schöne Günther, thanks, all these false positives are fixed in version 2016.04 :)