public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: gchare@google.com (Gabriel Charette)
To: reply@codereview.appspotmail.com, crowl@google.com,
	dnovillo@google.com,        gcc-patches@gcc.gnu.org
Subject: [pph] Remove protection for NAMESPACE_LEVEL being null when adding namespaces (issue4675069)
Date: Sat, 09 Jul 2011 10:15:00 -0000	[thread overview]
Message-ID: <20110709012935.B28761C3720@gchare.mtv.corp.google.com> (raw)

Diego, I know you're working in that area, but the bug fix I'm working on is kinda coming up to it too.

I realize you readded those two lines (in patch below) because of an ICE (although I still think they don't make sense, i.e. adding the bindings of a namespace to itself when they're already in there...). I checked and the only reason this fixes your ICE is because it goes through and calls varpool_finalize_decl for the names in that namespace bindings, the rest does absolutely nothing...

I know you're restructuring this right now, but just in case you're keeping some of that logic, I'm sharing my analysis on the matter now instead of patching it later...

Also as highlighted in this patch, I don't think we need to validate that NAMESPACE_LEVEL != NULL with the "if". It shouldn't be (I think... I even tested it with an empty namespace...), if anything maybe we want to assert, but not skip if it's NULL in my opinion.

Tested with bootstrap and pph regression testing.

Gab

2011-07-08  Gabriel Charette  <gchare@google.com>

	* gcc/cp/pph-streamer-in.c (pph_add_bindings_to_namespace):
	NAMESPACE_LEVEL should never be null for a namespace, removed check.

diff --git a/gcc/cp/pph-streamer-in.c b/gcc/cp/pph-streamer-in.c
index d78ee91..55f7e12 100644
--- a/gcc/cp/pph-streamer-in.c
+++ b/gcc/cp/pph-streamer-in.c
@@ -1168,8 +1168,7 @@ pph_add_bindings_to_namespace (struct cp_binding_level *bl, tree ns)
 	 Preserve it.  */
       chain = DECL_CHAIN (t);
       pushdecl_into_namespace (t, ns);
-      if (NAMESPACE_LEVEL (t))
-	pph_add_bindings_to_namespace (NAMESPACE_LEVEL (t), t);
+      pph_add_bindings_to_namespace (NAMESPACE_LEVEL (t), t);
     }
 }
 

--
This patch is available for review at http://codereview.appspot.com/4675069

             reply	other threads:[~2011-07-09  1:29 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-07-09 10:15 Gabriel Charette [this message]
2011-07-12 12:54 ` Diego Novillo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110709012935.B28761C3720@gchare.mtv.corp.google.com \
    --to=gchare@google.com \
    --cc=crowl@google.com \
    --cc=dnovillo@google.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=reply@codereview.appspotmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).