On Mon, 2021-07-12 at 22:07 +0530, Ankur Saini wrote: > > > > On 11-Jul-2021, at 11:19 PM, David Malcolm <dmalc...@redhat.com> > > wrote: > > > > On Sat, 2021-07-10 at 21:27 +0530, Ankur Saini wrote:
[...] > > > > > > - for the callstring patch, I created a patch file ( using git > > > format- > > > patch ) and sent it to patches mailing list (via git send email ) > > > and > > > CCed my mentor. > > > Although I received a positive reply from the command log (git send > > > email) saying the mail was sent , I didn’t received that mail ( > > > being > > > subscribed to the patches list ) regarding the same ( I sent that > > > just > > > before sending this mail ). > > > The mail should be sent from arse...@sourceware.org <mailto: > > > arse...@sourceware.org> <mailto: > > > arse...@sourceware.org <mailto:arse...@sourceware.org>> > > > > Thanks. > > > > I see the patch email in the list archives here: > > https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574888.html < > > https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574888.html> > > but for some reason it's not showing up in my inbox. I'm not sure > > why; > > I recently got migrated to a new email server and my filters are > > currently a mess so it could be a problem at my end; sorry if that's > > the case. > > > > Given that neither you nor I seem to have received the patch I wonder > > if anyone else received it? > > Then I think it’s better to attach patch file with the updates here > instead. FWIW I use "git send-email". Might be worth trying that again to see if it happens again, or if it was a one-time glitch. [...] > > > > > > If your editor supports it, it's easy to comply with a project's > > coding > > standards, otherwise it can be a pain. > > Oh, I see. This explains the weird indentation convention I was seeing > throughout the source. Actually my editor dynamically adjusts the width > of the tab depending on the style used in source file and due to some > reasons, it decided that it was 2 space wide here, this was leading to > some weird indentations throughout the source. > Well now it should be fixed, I manually adjusted it to be standard 8 > wide now and switched of converting tabs to spaces in my editor > settings. Well, it is 2 spaces wide, but using tabs to take the place of 8 spaces at a time when the indentation gets too large. [...] > > > > > > [...] > > > > > > From 95572742f1aaad1975aa35a663e8b26e671d4323 Mon Sep 17 00:00:00 > > > > 2001 > > > From: Ankur Saini <arse...@sourceware.org <mailto: > > > arse...@sourceware.org>> > > > Date: Sat, 10 Jul 2021 19:28:49 +0530 > > > Subject: [PATCH 2/2] analyzer: make callstring's pairs of > > > supernodes > > > statically allocated [GSoC] > > > > > > 2021-07-10 Ankur Saini <arse...@sourceware.org <mailto: > > > arse...@sourceware.org>> > > > > > > gcc/analyzer/ > > > * call-string.cc <http://call-string.cc/>: store a > > > vector of std::pair of supernode* instead of pointer to them > > > * call-string.h: create a typedef for "auto_vec<const > > > std::pair<const supernode *,const supernode *>*> m_supernodes;" to > > > enhance readibility > > > > ...and to avoid having really long lines! > > > > > * program-point.cc <http://program-point.cc/>: refactor > > > program point to work with new call-string format > > > > I think it's going to be much easier for me if you squash these two > > patches into a single patch so I can review the combined change. (If > > you've not seen it yet, try out "git rebase -i" to see how to do > > this). > > woah, this is magic ! > I always use to perform a soft reset ( git reset —soft <commit> ) and > commit in order to squash or reword my commits before, but never knew > we could change history locally like this, amazing : D I love "git rebase -i" and "git add -p"; together they make me look like a much better programmer than I really am :) [...] > > > > > > + typedef std::pair<const supernode *,const supernode *> element_t; > > > > Rather than a std::pair, I think a struct inside call_string like > > this > > would be better: rather than "first" and "second" we can refer to > > "m_caller" and "m_callee", which is closer to being self-documenting, > > and it allows us to add member functions e.g. "get_caller_function": > > > > class call_string > > { > > public: > > struct element_t > > { > > element_t (const supernode *caller, const supernode *callee) > > : m_caller (caller), m_callee (callee) > > { > > } > > > > function *get_caller_function () const {/*etc*/} > > function *get_callee_function () const {/*etc*/} > > > > const supernode *m_caller; > > const supernode *m_callee; > > }; > > > > }; > > > > [...snip...] > > > > which might clarify the code further. > > Instead of putting that struct inside the class, I declared it globally > and overloaded some basic operators ( “==“ and “!=“ ) to make it work > without having to change a lot how it is being handled in other areas > of source ( program_point.cc and engine.cc ). Fair enough, but calling it "element_t" seems too generic to me if it's going to be in global scope. I was suggesting to put it in call_string so that it's a call_string::element_t when referred to from global scope. > > > > > > > > > - I also added the example I was using to test the analyzer to > > > regression tests as "gcc/testsuite/gcc.dg/analyzer/call-via- > > > fnptr.c”. > > > > Great! Please add it to the patch. > > I am thinking to add it with later patches as this one only focuses on > changing the call_string only and doesn’t quite fix this bug. [...snip...] The patch is looking good. Some very minor nits: > +function * > +element_t::get_caller_function () const > +{ > + return m_caller->get_function(); Missing space before the open-parenthesis in the call to get_function. > +} > + > +function * > +element_t::get_callee_function () const > +{ > + return m_callee->get_function(); > +} Likewise here. Otherwise, looks good. How does this affect the testsuite results? Dave
