FPC and Lazarus coding style

[Okoba]

Hello,

Normally I do not look at the code of FPC or Lazarus, but today I was looking at some of the units, and it was hard for me to read the codes, especially the FPC. The code seems disturbing. It seems no spacing, format, or even case of keywords is written with a rule.

I wanted to ask how this is managed at the level of such big projects. Do professional developers not care about this stuff, or are these codes old and people have no time to clean them up or something I, as a normal developer, cannot understand?

Please note that I do not want to begin a discussion to say bad stuff about people who created such great projects that I use daily. I am just curious about the development style and needs of such projects that lead to this type of code.


Here is a sample form typinfo unit, you can see even a simple "if" is written as "if" or "If" or spacing looks wrong. And this is not part of the compiler; the compiler codes look much scarier to me with so much web of ifs.

Code: Pascal  [Select][+][-]

1. Function GetEnumValue(TypeInfo : PTypeInfo;const Name : string) : Integer;
2.  
3.   Var PS : PShortString;
4.       PT : PTypeData;
5.       Count : longint;
6.       sName: shortstring;
7.  
8. begin
9.   If Length(Name)=0 then
10.     exit(-1);
11.   sName := Name;
12.   PT:=GetTypeData(TypeInfo);
13.   Count:=0;
14.   Result:=-1;
15.  
16.   if TypeInfo^.Kind=tkBool then
17.     begin
18.     If CompareText(BooleanIdents[false],Name)=0 then
19.       result:=0
20.     else if CompareText(BooleanIdents[true],Name)=0 then
21.       result:=1;
22.     end
23.  else
24.    begin
25.      PS:=@PT^.NameList;
26.      While (Result=-1) and (PByte(PS)^<>0) do
27.        begin
28.          If ShortCompareText(PS^, sName) = 0 then
29.            Result:=Count+PT^.MinValue;
30.          PS:=PShortString(pointer(PS)+PByte(PS)^+1);
31.          Inc(Count);
32.        end;
33.      if Result=-1 then
34.        Result:=GetEnumeratedAliasValue(TypeInfo,Name);
35.    end;
36. end;
37.  

Sample from compiler. I can not even follow the parenthesis.

Code: Pascal  [Select][+][-]

1.  if (taicpu(p).condition <> C_None) and
2.         (taicpu(p).oper[0]^.typ = top_ref) and
3.         GetNextInstruction(p, hp1) and
4.         { Check individually instead of using MatchInstruction in order to save time }
5.         (hp1.typ = ait_instruction) and
6.         (taicpu(hp1).condition = C_None) and
7.         (taicpu(hp1).oppostfix = PF_None) and
8.         (taicpu(hp1).ops = 2) and
9.         (
10.           (
11.             (taicpu(hp1).opcode = A_MOVZ) and
12.             (taicpu(hp1).oper[1]^.val in [0, 1])
13.           ) or
14.           (
15.             (taicpu(hp1).opcode = A_MOV) and
16.             (getsupreg(taicpu(hp1).oper[1]^.reg) = RS_XZR)
17.           )
18.         ) and
19.         GetNextInstruction(hp1, hp2) and
20.         MatchInstruction(hp2, A_B, [PF_None]) and
21.         (taicpu(hp2).condition = C_None) and
22.         (taicpu(hp2).oper[0]^.typ = top_ref) and
23.         GetNextInstruction(hp2, hp3) and
24.         (hp3.typ = ait_label) and
25.         (tasmlabel(taicpu(p).oper[0]^.ref^.symbol) = tai_label(hp3).labsym) and
26.         GetNextInstruction(hp3, hp4) and
27.         { As before, check individually instead of using MatchInstruction in order to save time }
28.         (hp4.typ = ait_instruction) and
29.         (taicpu(hp4).condition = C_None) and
30.         (taicpu(hp4).oppostfix = PF_None) and
31.         (taicpu(hp4).ops = 2) and
32.         (taicpu(hp4).oper[0]^.reg = taicpu(hp1).oper[0]^.reg) and
33.         (
34.           (
35.             (taicpu(hp4).opcode = A_MOVZ) and
36.             (
37.               (
38.                 { Check to confirm the following:
39.                   - First mov is either "movz reg,#0" or "mov reg,xzr"
40.                   - Second mov is "movz reg,#1"
41.                 }
42.                 (
43.                   (taicpu(hp1).oper[1]^.typ = top_reg) { Will be the zero register } or
44.                   (taicpu(hp1).oper[1]^.val = 0)
45.                 ) and
46.                 (taicpu(hp4).oper[1]^.val = 1)
47.               ) or
48.               (
49.                 { Check to confirm the following:
50.                   - First mov is "movz reg,#1"
51.                   - Second mov is "movz reg,#0"
52.                 }
53.                 MatchOperand(taicpu(hp1).oper[1]^, 1) and
54.                 (taicpu(hp4).oper[1]^.val = 0)
55.               )
56.             )
57.           ) or
58.           (
59.             { Check to confirm the following:
60.               - First mov is "movz reg,#1"
61.               - Second mov is "mov reg,xzr"
62.             }
63.             (taicpu(hp4).opcode = A_MOV) and
64.             (getsupreg(taicpu(hp4).oper[1]^.reg) = RS_XZR) and
65.             MatchOperand(taicpu(hp1).oper[1]^, 1)
66.           )
67.         ) and
68.         GetNextInstruction(hp4, hp5) and
69.         (hp5.typ = ait_label) and
70.         (tasmlabel(taicpu(hp2).oper[0]^.ref^.symbol) = tai_label(hp5).labsym) then
71.         begin
72.           Invert := MatchOperand(taicpu(hp1).oper[1]^, 1); { if true, hp4 will be mov reg,0 in some form }
73.           if Invert then
74.             taicpu(p).condition := inverse_cond(taicpu(p).condition);
75.  
76.           tai_label(hp3).labsym.DecRefs;
77.  

[Martin_fr]

Well, I am not part of FPC, and hence can't give many details on those code samples. I am part of the Lazarus team though.

One generic remark I can provide: In some part code formatting, will always be something that happened over time. Well, unless you had strict automatic linters checking every commit from day one before it even gets committed.

Anyway, once you have it in, the cost of changing it (to match other code) is most often higher than the cost of having to read code that isn't exactly formatted as one expects.
The reason for this is, that old commits are part of the development. Things like  "git blame" or simple diffs between commits, are helpful tools in understanding why code exists, and what it does.
A pure formatting commit, could hit lots of lines, and git blame would require extra steps to find any commit that actually functionally changed a given bit of code.


In the Lazarus section we have different bits of code maintained by different people. And that leads to different styles, in each those section.

[Martin_fr]

I can not even follow the parenthesis.

Off topic, but the Lazarus 5.0 will have rainbow brackets.

[440bx]

Sample from compiler. I can not even follow the parenthesis.
 < code snippet >

Actually, as far as matching parentheses, that code is unusually good.  I've seen a lot worse than that.  At least in that snippet the opening and closing parentheses are easy to visually match (it also helps that both, Lazarus and the editor I use, match parentheses anyway.)

You're right that there doesn't seem to be a set of formatting rules that is strictly followed but, overall, while I wouldn't say it's great, it isn't that bad.

Off topic, but the Lazarus 5.0 will have rainbow brackets.

in that case, the absence of a pot of gold at the end of the bracket should be considered a bug.  Maybe that bug should be preemptively reported to ensure this new feature is implemented correctly. 

[LeP]

In Delphi there is this kind of help (see image attached), and for parenthesis you can select one and the corresponding will be highlighted (in blue, see second images).

If one code correctly, this kind of graphical help will not be necessary.

I see many developers that use notepad, word, edlin and countless other tools for writing code... then when you edit these sources in the Lazarus IDE, the formats don't work (for example, TABS or LF), and the code appears haphazard and unreadable (in Delphi is worse, really worse).

There have been other threads in this forum on these topics, and apparently it seems like a fairly common practice to skip the IDE and use third-party editing applications.

If this is the case, I don't think the code visibility problem will ever be solved.

[Martin_fr]

Off topic:

In Delphi there is this kind of help (see image attached), and for parenthesis you can select one and the corresponding will be highlighted

https://wiki.lazarus.freepascal.org/New_IDE_features_since#Outline_for_Pascal_Sources
And matching bracket at caret highlight we also have (just set a color in the settings)
And we also have highlight of begin/end pairs at caret, and a ton of other....

the formats don't work (for example, TABS or LF)

Don't know what the LF issues are. But tabs can be set any way you want. Including elastic.

[LeP]

Off topic:
https://wiki.lazarus.freepascal.org/New_IDE_features_since#Outline_for_Pascal_Sources
And matching bracket at caret highlight we also have (just set a color in the settings)

Thanks, I didn't know about those.

[Martin_fr]

Back on the original question

a simple "if" is written as "if" or "If" or spacing looks wrong

The upper/lower wouldn't bother me. Not for keywords anyway, minimal for vars/idents if/when inconsistent.

The indent (e.g. the one "else") is indeed unfortunate. If that was code I would work on (which it isn't) then I would consider fixing it, if and likely only if, I had other functional changes in the same code block to make.


The compiler code with the brackets... Despite the many all lowercase names, it is actually very readable, and the format/layout is helpful, even if I would come from a different style (I don't, but if).

The issues here is not so much the code, but the enormous underlying data structures, that aren't just self explaining. Without knowing how this (my guess) node-tree of instructions is build, the code cannot be understood....
And some of the abbreviations aren't self explaining either.

So to understand this code, there is some initial read up needed, to get all that context. But that is not "code formatting" related.

You could argue (but even that isn't formatting, despite affecting it), that the long if conditions could be moved into groups in a serious of methods on the target object.

Code: Pascal  [Select][+][-]

1. or
2. taicpu.is_operater_foobar
3. or
4. ...

It affects formatting by reducing long statements. But it primarily is code-design. By moving functionality into classes or functions....

And, as I don't really have done my due diligence in reading up and understanding the code => this may even be a proposal to make it worse.

I have in some of my code similar issues. And in some of it, I will probably keep it.

[Curt Carpenter]

For what it's worth, I've found that reformatting a program to fit my own personal style quirks is one of the best ways to study it, at least initially.  It forces me to go over the code line by line and helps me be patient with the learning process.   Note too that where "style" is concerned, case sensitive languages and  programs drive me nuts -- a hangover from ancient habits that began with punched cards and teletype I suppose.

[Khrys]

I've found that reformatting a program to fit my own personal style quirks is one of the best ways to study it, at least initially.

I've had the same experience; I think that manually reformatting forces you to mentally parse at least the control flow structure, which is already better than just skimming the code.

Note too that where "style" is concerned, case sensitive languages and  programs drive me nuts

I'm the complete opposite - case insensitive languages drive me nuts because of how inconsistent (actually, I'd rather call it sloppy) your colleagues are allowed to be 

[Joanna]

I agree with curt. If you don’t like the style change it for yourself.

[Thaddy]

The compiler code itself uses indeed a different, to many unfamiliar way, of naming and formatting rules but it is standardized dating from tp days or earlier. Most- but - not all code elsewhere uses basically the same convention as Delphi does.

[Zvoni]

The indent (e.g. the one "else") is indeed unfortunate. If that was code I would work on (which it isn't) then I would consider fixing it, if and likely only if, I had other functional changes in the same code block to make.

Which "else" would that be?
the only "fishy" formatting i see is Line 18 to Line 21, which indents to the same level of the "Begin" in Line 17 resp. the "end" in Line 22

[Martin_fr]

The indent (e.g. the one "else") is indeed unfortunate. If that was code I would work on (which it isn't) then I would consider fixing it, if and likely only if, I had other functional changes in the same code block to make.

Which "else" would that be?
the only "fishy" formatting i see is Line 18 to Line 21, which indents to the same level of the "Begin" in Line 17 resp. the "end" in Line 22

Line 23.

The else has no indent at all. But all other code in the procedure's begin/end is indented.

About line 18 to 21: That is a common style. The "begin (and it's end) are themself indented. The code they enclose is not further indented, as the begin/end already is. I.e., the begin/end is part of the indented code.

This is logical if you have "if then" followed by an indented statement on the next line(s). The begin/end is one compound statement. It gets indented for the "if".

[Okoba]

@Martin_fr thank you for the input.
The link to the wiki was new to me, I enabled the rainbow colors. Btw do you know how to enable the spell checker? I can not find the SynEditSpellCheckerDsgn package. I am using the Trunk.