Code::Blocks Forums

Developer forums (C::B DEVELOPMENT STRICTLY!) => Development => Compiler Framework Redesign => Topic started by: rickg22 on June 16, 2007, 10:26:09 am

Title: New compiler line type: cltInfo
Post by: rickg22 on June 16, 2007, 10:26:09 am
I just discovered a wonderful feature in C::B. It's called "Begin word" search :-P

With it i could finally add the compiler line type "cltInfo" so no info lines will count as warnings. I uploaded a PATCH to berlios.

The reason I didn't commit, is that the compiler, being a critical part of C::B, cannot afford unrevised commits. However, I did test the patch (I compiled C::B, and had to rebuild the entire project around 3 times to make sure it worked properly) and it works wonders.

I added a new info line: "In member function .....:" so that those STL errors can be understood more clearly.

Mind checking it out, everyone??

http://developer.berlios.de/patch/?func=detailpatch&patch_id=2057&group_id=5358

Another thing that worries me, is that saved custom compiler regexes will be altered by the addition of this new line type. I hope it's a marginal case, since not all beta testers will mess up with the compiler parsing... i hope :| - and this is another reason why I didn't commit.

Yiannis, your stance?
Title: Re: New compiler line type: cltInfo
Post by: MortenMacFly on June 16, 2007, 10:43:43 am
With it i could finally add the compiler line type "cltInfo" so no info lines will count as warnings.
Nice Rick! In r4094 I asked myself the question: Why do we actually don't have a cltInfo? A few days later you come up with a patch... I'm shivering... ;-) Anyways - I'm currently compiling C::B with that one incorporated. Thanks!
With regards, Morten.

Edit: First thing I noticed: The changes you made for CompilerMinGW are missing in CompilerGNUARM and CompilerGNUAVR (these are also GNU GCC based) - I'll remember that if we decide to commit the changes.
In addition I realised that in GNUARM the "Instantiated from here" line (as it is currently) has the resource compiler error twice... looks like a copy & paste error (a forgotten linefeed in that line).
Title: Re: New compiler line type: cltInfo
Post by: rickg22 on June 16, 2007, 08:29:53 pm
Actually there should NOT be an "instantiated from here" regex. Just "instantiated from" because i've gotten errors which mention a class and not just "here". This has to be corrected for all compiler sets.

BTW, here's a small snapshot of a compilation result (not build log, but build MESSAGES) after the patch.

:: === Code::Blocks, sdk ===
G:\MinGw\bin\..\lib\gcc\mingw32\3.4.5\..\..\..\..\include\c++\3.4.5\bits\stl_list.h:: In constructor `InfoWindow::InfoWindow(const wxString&, const wxString&, unsigned int, unsigned int)':
G:\MinGw\bin\..\lib\gcc\mingw32\3.4.5\..\..\..\..\include\c++\3.4.5\bits\stl_list.h:435: warning: '__p' might be used uninitialized in this function
G:\MinGw\bin\..\lib\gcc\mingw32\3.4.5\..\..\..\..\include\c++\3.4.5\bits\stl_list.h:: In constructor `InfoWindow::InfoWindow(const wxString&, const wxString&, unsigned int, unsigned int)':
G:\MinGw\bin\..\lib\gcc\mingw32\3.4.5\..\..\..\..\include\c++\3.4.5\bits\stl_list.h:435: warning: '__p' might be used uninitialized in this function
:: === Build finished: 0 errors, 2 warnings ===

See? Just 2 warnings, and now we get more info :)
Title: Re: New compiler line type: cltInfo
Post by: MortenMacFly on June 18, 2007, 10:37:38 pm
Actually there should NOT be an "instantiated from here" regex.
It wasn't - don't worry, that was a typo in my post. Hence I have added the same support for the other two GCC compilers in the same style. After testing the patch for the last couple of days I don't see any negative side effects. It looks really nice to have info separated from warnings in the log. So I wouldn't have any objections from my point of view... what about the others testing this?!
With regards, Morten.
Title: Re: New compiler line type: cltInfo
Post by: rickg22 on June 19, 2007, 03:39:07 am
the only problem would arise (I think) with people with custom compilers, upgrading from previous installations of Code::Blocks. They'd have to change their compilers, which is why I'm unsure of making this change into the trunk. Perhaps we should add a new popup message (InfoWindow)?
Title: Re: New compiler line type: cltInfo
Post by: MortenMacFly on June 19, 2007, 07:48:47 am
the only problem would arise (I think) with people with custom compilers, [...]
I am such (I have quite some custom additional "native" and copy-based compilers) but I don't really see a big issue. Isn't the only thing that can happen that people still see warnings, where information maybe better... so what?! I guess I'm missing something...?!
With regards, Morten.
Title: Re: New compiler line type: cltInfo
Post by: rickg22 on June 19, 2007, 07:56:38 am
Let's see what Don Corleone says  :P
Title: Re: New compiler line type: cltInfo
Post by: mandrav on June 19, 2007, 09:10:31 am
Let's see what Don Corleone says  :P

Quote from: top of sdk/compiler.cpp
// version of compiler settings
// when this is different from what is saved in the config, a message appears
// to the user saying that default settings have changed and asks him if he wants to
// use his own settings or the new defaults
const wxString CompilerSettingsVersion = _T("0.0.1");

Just make the change (if it doesn't break anything else) and pump that number a bit.
Title: Re: New compiler line type: cltInfo
Post by: rickg22 on June 20, 2007, 01:40:09 am
Thanks! That's what I was looking for. I'll commit asap.

Update: DONE! :-)
Title: Re: New compiler line type: cltInfo
Post by: killerbot on June 20, 2007, 08:50:41 am
everybody take care, I think a serious issue might be introduced here !!

When you have a look at default.conf, you will see that the regexes for parsing errors/warnings all are of a certain type.
Example : Compiler warning of dmd, is of type int="1" (actually on the 2 systems I checked it was at 2, old bug ???).
Now this 1 resembles something :
Quote
enum CompilerLineType
{
    cltNormal,
    cltWarning,
    cltError
};


But because the enum has been INCORRECTLY changed to :
Quote
enum CompilerLineType
{
    cltNormal,
    cltInfo,
    cltWarning,
    cltError
};

That means the warning regex is now threated as of type info, and so will the error be threated as warning ...

Never ever insert values in an enum that is exported as an int !!!!!!!!!

Gonna boot up my laptop and adjust this, the cltInfo should be the last one !!!
And even then I am not sure if all compilers will correctly adjust ???
Title: Re: New compiler line type: cltInfo
Post by: mandrav on June 20, 2007, 09:55:08 am
Quote
Never ever insert values in an enum that is exported as an int !!!!!!!!!

Gonna boot up my laptop and adjust this, the cltInfo should be the last one !!!
And even then I am not sure if all compilers will correctly adjust ???

Nice catch killerbot :).
Initialize cltNormal to 0 too, so we have predefined numbers...
Title: Re: New compiler line type: cltInfo
Post by: MortenMacFly on June 20, 2007, 10:39:06 am
everybody take care, I think a serious issue might be introduced here !!
Never ever insert values in an enum that is exported as an int !!!!!!!!!
I thought that's why the compiler version was changed -> thus the settings are reset and refreshed... isn't that why Rick was asking for it?! I mean: It really works on my PC, I see the correct colouring for each type even with the enum as it is now. In the end this compiler RegEx settings use the enum, right...?! (What am I missing to understand?!)
With regards, Morten.
Title: Re: New compiler line type: cltInfo
Post by: mandrav on June 20, 2007, 11:24:26 am
everybody take care, I think a serious issue might be introduced here !!
Never ever insert values in an enum that is exported as an int !!!!!!!!!
I thought that's why the compiler version was changed -> thus the settings are reset and refreshed... isn't that why Rick was asking for it?! I mean: It really works on my PC, I see the correct colouring for each type even with the enum as it is now. In the end this compiler RegEx settings use the enum, right...?! (What am I missing to understand?!)
With regards, Morten.

The problem is if the user decides NOT to use the new settings. His old settings will be wrong then...
Title: Re: New compiler line type: cltInfo
Post by: MortenMacFly on June 20, 2007, 11:31:08 am
The problem is if the user decides NOT to use the new settings. His old settings will be wrong then...
Oh yes... stupid me... that's an issue, in fact... :oops:
Title: Re: New compiler line type: cltInfo
Post by: rickg22 on June 20, 2007, 05:55:34 pm
Initialize cltNormal to 0 too, so we have predefined numbers...

You mean it's initialized to ONE???  :shock:

Ok, I'll update it and pump up the version (again) to 0.0.3. :P Just a last question. Should cltInfo be the second, or the last? Because in other files i assummed that cltWarning will always be greater than cltInfo.
Title: Re: New compiler line type: cltInfo
Post by: MortenMacFly on June 20, 2007, 06:00:55 pm
Ok, I'll update it and pump up the version (again) to 0.0.3.
Be careful: killerbot did already some change in r 4115.

:P Just a last question. Should cltInfo be the second, or the last? Because in other files i assummed that cltWarning will always be greater than cltInfo.
I believe the ">=" stuff won't work and I guess it's really better to use a switch case construct... what do you think?

With regards, Morten.
Title: Re: New compiler line type: cltInfo
Post by: mandrav on June 20, 2007, 06:03:41 pm
Initialize cltNormal to 0 too, so we have predefined numbers...

You mean it's initialized to ONE???  :shock:

No, I mean you don't really know what number the compiler will use for the first enumerator. It should be 0 but you don't know it for sure, and certainly not if you use a different-than-gcc compiler.
So, what I 'm saying is initialize the first enumerator to 0 and everything will be well defined.
Title: Re: New compiler line type: cltInfo
Post by: thomas on June 20, 2007, 06:28:41 pm
No, I mean you don't really know what number the compiler will use for the first enumerator. It should be 0 but you don't know it for sure, and certainly not if you use a different-than-gcc compiler.
Hmm actually that's in the C++ standard... but it certainly does not hurt to explicitely set it to 0. First, to account for broken compilers, and second for human readers who might possibly expect something else :)


I believe the ">=" stuff won't work and I guess it's really better to use a switch case construct... what do you think?
I use such constructs every day, they work nicely (as it happens, the new logging system distinguishes info from error like this too). enum values start at 0 and are incremented by 1 unless you tell the compiler something different. Thus, by properly ordering the enums (which is sensible, anyway), you can determine the "general nature" of a "thing" very easily and efficiently.

This btw. is one of the few things that are implemented really, really well in wxWidgets. On all stock items and all items created with defaults, you can tell from an item's (control, font, cursor, colour, language, encoding...) ID what it is.
Well, you should of course not do it for compatibility's sake, but the toolkit can tell what "unknown thing with ID foo" is, and that's pretty darn cool.
Title: Re: New compiler line type: cltInfo
Post by: killerbot on June 20, 2007, 06:37:45 pm
in the enum the first entry is 0, C/C++ does that, but we added it explicitly to make it more standing out ;-)

DO NOT change the order of the enum anymore, I fixed it. New enum values have to be added to the end, don't insert !!!!
Title: Re: New compiler line type: cltInfo
Post by: MortenMacFly on June 20, 2007, 06:43:03 pm
I use such constructs every day, they work nicely (as it happens, the new logging system distinguishes info from error like this too).
Sure you can use it if it makes sense. But after the latest change the order of the items will be:
Code
enum CompilerLineType
{
  cltNormal = 0,
  cltWarning,
  cltError,
  cltInfo
}
Tell me how you get the same functionality in those cases with >=???
With regards, Morten.
Title: Re: New compiler line type: cltInfo
Post by: killerbot on June 20, 2007, 06:57:15 pm
please please, don't reorder. The int values are being exported, you can avoid to break stuff, why break it, because it would be nice to have in the enum first  info, then warning and then error.

The next time there's something else then info, warning, error. Then what ? Reorder and break again.
Be explicit in switch, case :  code is much easier to read and less error prone.
Title: Re: New compiler line type: cltInfo
Post by: rickg22 on June 20, 2007, 08:22:35 pm
OK Killerbot. I won't touch it. Just make sure you've replaced my >= with ifs/ cases. (You can do a revision graph to see which lines i had changed).
Title: Re: New compiler line type: cltInfo
Post by: killerbot on June 20, 2007, 09:51:55 pm
A little recap
Starting this analysis with the old code before the changes of today.
From the GUI where you can add the types you want and you can select a line type :
In a combo box : Type :
  Info
  Warning
  Error

so I guess the SaveRegexDetail : rs.lt = (CompilerLineType)wxComboBox:GetSelection() would return for the above entries
  0
  1
  2

Now when we look at the enum :
Code
enum CompilerLineType
{
    cltNormal
    cltWarning,
    cltError
};

We see that info line types were mapped to cltNormal (0), and warning to cltWarning (1), and the error to cltError (2).

So all is well.

However if we want to treat the info separate from normal in the code, we should expand the enum. And because the number value of the enum is in the default.conf file for the compilers, we should expand it at the end. Not insertion, because that will cause a shift and things will be misinterpreted.

So we get :
Code
enum CompilerLineType
{
    cltNormal
    cltWarning,
    cltError,
    cltInfo,
};

Now we need to adjust also the gui part. There we have to make sure that an info line (type) ends up being   a cltInfo.
However it used to map onto a cltNormal. So here we surely change things and some first side effects in certain configurations might become visible.

Question here is : do we also add Normal to the combo box (?), so 4 choices, reflecting the 4 values of the enum. And if we want to do it with a simple cast like the original code, we need something like this :

Code
<content>
<item>Normal</item>
<item>Warning</item>
<item>Error</item>
<item>Info</item>

Let's do that for now. One might ask why add a regex if I want it to be just normal (normal things --> don't do anything specific in the code for those ...).
But on the other hand, we have a better match then.

Consequence : people already having "info entries" which were treated as "normal line types" will continue to see them in the GUI of the advanced settings as Normal and no longer as Info. So if you really want those to be treated as Info (whatever that means : see below) will have to change this through the gui.

So moving on to the next big question. How to deal with info's, now we conider them to be different from 'Normal'.
We have a and continue to have :
1) errors are shown in red and show up in the build messages
2) warnings are shown in blue and show up in the build messages
3) normal shown in black and don't show up in the build messages.

Now what is an info ?? For example in the Windriver compiler, an info is actually more of a warning. Maybe a less severe warning, but I have to say some of that compilers infos were pure warnings for other compilers, and that's the same opinion I had with those I cam across.

Todays changes had put the info in +- category 3) .
I feel they should be treated as warnings. Code should be error free, warning free, and info free ;-)

But let's keep the changes made today for mow. Feel free to tell your opinion bout this.

So we should change a ew more  lines of code (compiler.gcc) :
 - log to the build messages as long as the type isn't normal  (AddOutputLine)
 - but show it in the build messages in the system color for text (+- black) (like normal line type)  (LogWarningOrError)
 - in the log file (html) choose a correct colored font for it (LogMessage) ==> here the code was +- okin the past but certainly not correct, because a font tag was opened for error/warning or when forceErrorColour BUT it was only closed when it was not of type 'normal'. Obvious that when forceErrorColour was true and it was of type normal, that the tag would never be closed.

The line should have said, and should now, since info is treated colorwise as normal : close the font tag if 'error or warning or forcedErrorColour'.


So i will commit because of this story :
- xrc with the resynched entries
- compilergcc.cpp for that font tag closing
Title: Re: New compiler line type: cltInfo
Post by: MortenMacFly on June 20, 2007, 10:15:34 pm
I feel they should be treated as warnings. Code should be error free, warning free, and info free ;-) [...] Feel free to tell your opinion bout this.
As you have stated already, just to underline:
Normal = default message usually of no great importance at all -> build log
Info = message that better be put into build msgs because it's not a default message
Warning = message that better has to be put to the build msgs because something is wrong
Error = message that definitely has to be put to the build msgs because something went wrong
So I feel the Info category fits well in what the compiler tries to tell. But I agree totally with you: Compilation should also be "Info" free.

Coming to the enum / GUI value: I personally would like to see the ordering as Rick did it in the first place in code and the the same for the GUI. In daily live most of the people will click "OK to change settings" if the appropriate C::B warning has appeared (at least that's what happened at my work... ;-)). Having the enum sorted by importance has several pros imho. So here are my two cents: Ignore backward compatibility this time. :P Again: That's only my opinion.

With regards, Morten.