On 25/3/21 11:54 am, Joel Sherrill wrote: > > > On Wed, Mar 24, 2021, 7:28 PM Chris Johns <chr...@rtems.org > <mailto:chr...@rtems.org>> wrote: > > > > On 25/3/21 6:54 am, Joel Sherrill wrote: > > > > > > On Wed, Mar 24, 2021 at 2:42 PM Gedare Bloom <ged...@rtems.org > <mailto:ged...@rtems.org> > > <mailto:ged...@rtems.org <mailto:ged...@rtems.org>>> wrote: > > > > On Wed, Mar 24, 2021 at 1:35 PM Joel Sherrill <j...@rtems.org > <mailto:j...@rtems.org> > > <mailto:j...@rtems.org <mailto:j...@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 _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
