daily report on extending static analyzer project [GSoC]
David Malcolm
dmalcolm@redhat.com
Wed Jul 14 23:07:12 GMT 2021
On Mon, 2021-07-12 at 22:07 +0530, Ankur Saini wrote:
> >
> > On 11-Jul-2021, at 11:19 PM, David Malcolm <dmalcolm@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 arsenic@sourceware.org <mailto:
> > > arsenic@sourceware.org> <mailto:
> > > arsenic@sourceware.org <mailto:arsenic@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 <arsenic@sourceware.org <mailto:
> > > arsenic@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 <arsenic@sourceware.org <mailto:
> > > arsenic@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
More information about the Gcc
mailing list