Making Covoar More C++

[Chris Johns]

On 25/3/21 11:54 am, Joel Sherrill wrote:
> 
> 
> On Wed, Mar 24, 2021, 7:28 PM Chris Johns <chrisj at rtems.org
> <mailto:chrisj at rtems.org>> wrote:
> 
> 
> 
>     On 25/3/21 6:54 am, Joel Sherrill wrote:
>     >
>     >
>     > On Wed, Mar 24, 2021 at 2:42 PM Gedare Bloom <gedare at rtems.org
>     <mailto:gedare at rtems.org>
>     > <mailto:gedare at rtems.org <mailto:gedare at rtems.org>>> wrote:
>     >
>     >     On Wed, Mar 24, 2021 at 1:35 PM Joel Sherrill <joel at rtems.org
>     <mailto:joel at rtems.org>
>     >     <mailto:joel at rtems.org <mailto:joel at rtems.org>>> wrote:
>     >     >
>     >     > Hi
>     >     >
>     >     > There has been a lot of talk about making covoar use more C++
>     >     > features. It seems to be an issue on every patch. I almost
>     >     > replied to Gedare's comment at the bottom of a patch
>     >     > but decided it needed another thread:
>     >     >
>     >     > "I still struggle reviewing this codebase, in part because it is C+C++
>     >     > (TM) and in part because I'm not so proficient in C++ to make concrete
>     >     > recommendations how to write this better. I think, if the goal is
>     >     > eventually to make this more C++ like code, then new code coming in
>     >     > should aim to move the needle in that direction rather than continue
>     >     > to propagate the old ways of doing."
>     >     >
>     >     Thanks for taking this on.
> 
> 
> Alex's willing to make some of the changes. It's just a matter of having a
> baseline so when we make changes they can be tested thoroughly. We now have
> about a half dozen architectures producing reports so it is a good baseline.
> 
> I was actually thinking that once all of these are merged, it might be a good
> place to tag because any git bisect might want to come back to here.
> 
>     >
>     >     > I personally do NOT want to see changes to C++ in one leaf class and
>     >     > the other architectures not get the same changes. I would prefer to see
>     >     > all these corrections and base changes in the same style with limited
>     >     > changes to C-isms. I'm not opposed to the changes but let's take the
>     >     > Target class one. There are multiple target classes. Changing one
>     >     > independent of the others isn't a good idea.
>     >     >
>     >     This is reasonable to me.
>     >
>     >     > I'd like to see us get a working baseline in and then do something like
>     >     > sweep std::string through Target* as a single patch. This is easier to
>     >     > test and review since it would only be C string to std::string. Perhaps
>     >     > switch to C++ output a file at a time. Redo the report output. Etc.
>     >     > Discrete chunks instead of piecemeal.
>     >     >
>     >     > Covoar has spent years broken and some is from changing working
>     >     > things to do things "a better way" with no baseline to check against.
>     >     > We need to get a baseline.
>     >     >
>     >     > Please. Let's get a working baseline and then approach this more
>     >     > methodically. No one is going to suffer from seeing a C string a little
>     >     > while longer. :)
>     >     >
>     >     I'm fine, as long as there is a plan in place and some clear
>     >     directions. It would help to have tickets to organize the path
>     >     forward.
> 
> 
> +1
> 
> One issue is the order of the changes. I at first thought about making the
> string changes and then realized that we would end up having C printf and have
> to temporarily add c_str a lot. It likely makes more sense to sweep all the
> output first and then switch to C++ strings.
> 
> I have also asked Alex to put together some diagrams so we can discuss this
> better. I think there is some data flow and delivered inheritance in this
> program that is not being understood by everyone.
> 
> Plus Alex and I talked earlier this evening and think we have a reasonable path
> to greatly speed it up without massive overhauls.
> 
>     >
>     >     I'm willing to oblige continued use of C'ism for now, but I want to
>     >     know the plan and maybe a deadline of sorts by which I can start to be
>     >     picky again :) I don't like to be in limbo.
>     >
>     >
>     > I'd love to have a deadline but I can't guarantee how long Alex can
>     > work on it. But unless he gets pulled, he can pick on this for a while.
> 
>     Joel, I pushed a number of changes to move covoar towards C++ back when I pushed
>     in the ELF and DWARF support. The C++ nature of the interfaces I brought in from
>     the tool kit required some refactoring. I have not seen much action since then
>     so Gedare's question seems reasonable.
> 
> 
> Yes you did and because we did not review the reports, there was a backlog of
> issues that had to be fixed. Some from this change some from other changes. No
> blame. I just want a better baseline this time.

Yes there issues, I made the change but did limited testing given it was not
funded and I want to see the code move to a faster means of managing ELF files.

>     The need for this code to be C++ and not a mix comes down to what you want to
>     see happen. There is no real need to move the code closer to C++ other than
>     improving the technical quality and that is about the life of the code in the
>     project. If you are willing to look after the code as is then I am fine with
>     that. If you would like to see the code move to C++ and away from C then can
>     also happen. It would be an interesting way to learn more about C++.
> 
>     I find the code hard to work on because I do not know if I am looking at C and
>     needing to use C solutions or C++ and I should be pulling on C++ threads and
>     where they go. I also see this is an issue for those working on the code. In
>     some recent patches I pointed out new code being call from C++ code with C++
>     objects that was written in C. I pointed this out and the next patch was fine.
> 
> 
> There really isn't any straight C code in here. Just C-strings and C style IO. 

I consider strings and IO low hanging fruit that mess with patch reviews. In
terms of C vs C++ I am looking at the use of pointers. A raw pointer is a tool
in C and can be avoided in C++ and there are good reasons to do that. I am not
sure debating this here will help and I think a discussion about C++ in general
would be more constructive.

>     > My guess is that C string to std::string is probably a good pass by
>     > itself since method signatures may change.
> 
>     I think anything you feel can be changed and is in reach is welcome.
> 
>     > There isn't much file input but that could be a pass by itself
>     > along the way,
>     >
>     > Then sweep output one file at a time leaving reporting for last as
>     > a batch.
>     >
>     > Do you see an order?
> 
>     Maybe we organise a group session online where we all look over the code
>     together and discuss various aspects of the architecture and what it means from
>     a strictly C vs C++ point of view. Any change like this cannot happen in a
>     vacuum and I believe C++ is more of a taught language than C. C styles are
>     easier to pick up  by reading code. My introduction to C++ was in the mid-90's
>     with a Rational training course and documentation. It takes time to relearn C
>     solutions you know and have at hand in C++.
> 
> This sounds good. 
> 
>     I have no idea what we would need to hold such a session and who would be
>     interested but I am happy to be there and be part of it.
> 
> Probably just a Google meeting is fine

I am fine with any one turning up as my time would be unfunded. I suppose it
depends on who would be interested?

Chris