Re: [PATCH] Fix memory leaks in tree-ssa-uninit.c

[Martin Liška]

On 11/20/2015 12:15 PM, Martin Liška wrote:

On 11/20/2015 03:14 AM, Bernd Schmidt wrote:

BTW, I'm with whoever said absolutely no way to the idea of making automatic 
changes like this as part of a commit hook.

I think the whitespace change can go in if it hasn't already, but I think the 
other one still has enough problems that I'll say - leave it for the next stage 
1.

@@ -178,8 +173,9 @@ warn_uninitialized_vars (bool warn_possibly_uninitialized)

    FOR_EACH_BB_FN (bb, cfun)
      {
-      bool always_executed = dominated_by_p (CDI_POST_DOMINATORS,
-                         single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)), bb);
+      bool always_executed
+    = dominated_by_p (CDI_POST_DOMINATORS,
+              single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)), bb);
        for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
      {

Better to pull the single_succ into its own variable perhaps?

@@ -1057,7 +1039,8 @@ prune_uninit_phi_opnds_in_unrealizable_paths (gphi *phi,
           *visited_flag_phis = BITMAP_ALLOC (NULL);

         if (bitmap_bit_p (*visited_flag_phis,
-                SSA_NAME_VERSION (gimple_phi_result (flag_arg_def))))
+                SSA_NAME_VERSION (
+                  gimple_phi_result (flag_arg_def))))
           return false;

         bitmap_set_bit (*visited_flag_phis,

Pull the gimple_phi_result into a separate variable, or just leave it unchanged 
for now, the modified version is too ugly to live.

         bitmap_clear_bit (*visited_flag_phis,
-                SSA_NAME_VERSION (gimple_phi_result (flag_arg_def)));
+                SSA_NAME_VERSION (
+                  gimple_phi_result (flag_arg_def)));
         continue;
       }

Here too.

-  all_pruned = prune_uninit_phi_opnds_in_unrealizable_paths (phi,
-                                 uninit_opnds,
-                                 as_a <gphi *> (flag_def),
-                                 boundary_cst,
-                                 cmp_code,
-                                 visited_phis,
-                                 &visited_flag_phis);
+  all_pruned = prune_uninit_phi_opnds_in_unrealizable_paths
+    (phi, uninit_opnds, as_a<gphi *> (flag_def), boundary_cst, cmp_code,
+     visited_phis, &visited_flag_phis);

I'd rather shorten the name of the function, even if it goes against the spirit 
of the novel writing month.

-      if (gphi *use_phi = dyn_cast <gphi *> (use_stmt))
-    use_bb = gimple_phi_arg_edge (use_phi,
-                      PHI_ARG_INDEX_FROM_USE (use_p))->src;
+      if (gphi *use_phi = dyn_cast<gphi *> (use_stmt))
+    use_bb
+      = gimple_phi_arg_edge (use_phi, PHI_ARG_INDEX_FROM_USE (use_p))->src;
         else
       use_bb = gimple_bb (use_stmt);

There are some changes of this nature and I'm not sure it's an improvement. 
Leave them out for now?

@@ -2345,8 +2309,8 @@ warn_uninitialized_phi (gphi *phi, vec<gphi *> *worklist,
       }

     /* Now check if we have any use of the value without proper guard.  */
-  uninit_use_stmt = find_uninit_use (phi, uninit_opnds,
-                     worklist, added_to_worklist);
+  uninit_use_stmt
+    = find_uninit_use (phi, uninit_opnds, worklist, added_to_worklist);

     /* All uses are properly guarded.  */
     if (!uninit_use_stmt)

Here too.

@@ -2396,12 +2359,24 @@ public:
     {}

     /* opt_pass methods: */
-  opt_pass * clone () { return new pass_late_warn_uninitialized (m_ctxt); }
-  virtual bool gate (function *) { return gate_warn_uninitialized (); }
+  opt_pass *clone ();
+  virtual bool gate (function *);

This may technically violate our coding standards, but it's consistent with a 
lot of similar cases. Since coding standards are about enforcing consistency, 
I'd drop this change.

   namespace {

   const pass_data pass_data_early_warn_uninitialized =
   {
-  GIMPLE_PASS, /* type */
+  GIMPLE_PASS,               /* type */
     "*early_warn_uninitialized", /* name */
-  OPTGROUP_NONE, /* optinfo_flags */
-  TV_TREE_UNINIT, /* tv_id */
-  PROP_ssa, /* properties_required */
-  0, /* properties_provided */
-  0, /* properties_destroyed */
-  0, /* todo_flags_start */
-  0, /* todo_flags_finish */
+  OPTGROUP_NONE,           /* optinfo_flags */
+  TV_TREE_UNINIT,           /* tv_id */
+  PROP_ssa,               /* properties_required */
+  0,                   /* properties_provided */
+  0,                   /* properties_destroyed */
+  0,                   /* todo_flags_start */
+  0,                   /* todo_flags_finish */
   };

Likewise. Seems to be done practically nowhere.

   class pass_early_warn_uninitialized : public gimple_opt_pass
@@ -2519,14 +2491,23 @@ public:
     {}

     /* opt_pass methods: */
-  virtual bool gate (function *) { return gate_warn_uninitialized (); }
-  virtual unsigned int execute (function *)
-    {
-      return execute_early_warn_uninitialized ();
-    }
+  virtual bool gate (function *);
+  virtual unsigned int execute (function *);

Likewise.


Bernd

Hi.

Enhanced patch should cover all notes pointed in the previous email.

Martin

PING.

Is the patch still candidate to be merged in current stage3, or should I leave 
it to the next stage1?
What about the first patch or the patch, where I just applied replacement of 
whitespaces?

Thanks,
Martin