This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [C++ PATCH] Enforce extern "Java" aggregates to be heap allocated (PR c++/30293, c++/30294)


Jakub Jelinek wrote:

> All of the listed attributes are also true of C++, though C++ has
> extra features (for example in C++ objects may be allocated not just
> on the heap, but also statically or in a local stack frame).

This looks OK, but I've picked some nits.  I'd also like one of the Java
maintainers to sign off on the concept, just to be sure that we don't
break Java CNI code in the field.

> +	  if (TYPE_FOR_JAVA (type) && IS_AGGR_TYPE (type))
> +	    {
> +	      tree jclass;
> +	      /* Allow libjava/prims.cc define primitive classes.  */
> +	      if (init != NULL_TREE
> +		  || (jclass
> +		      = IDENTIFIER_GLOBAL_VALUE (get_identifier ("jclass")))
> +		     == NULL_TREE

Please move the assignment to jclass outside the conditional.  If you're
trying to avoid the get_identifier in the !init case, then (a) this is
not fast-path code as there are few declarations of extern "Java"
classes, and (b) you could just put "init &&" on the first line above,
if I'm reading correctly.

> +		  || TYPE_MAIN_VARIANT (type)
> +		     != TYPE_MAIN_VARIANT (TREE_TYPE (TREE_TYPE (jclass))))

You should parenthesize this last clause: (TYPE_MAIN .. != TYPE_MAIN
...) so that using Emacs auto-indent doesn't reformat the line.  Also,
would this be clearer if you used same_type_ignoring_top_level_qualifiers?

> +		error ("Java object %qD must be heap allocated", decl);

To be grammatical, that should be "heap-allocated".  (Use a hyphen, not
a space.)  But, "heap-allocated" is jargon; we should use either C++ or
Java terminology.  So, I would suggest:

  Java object %qD not allocated with %<new%>

(I'm not sure if %<...%> is the right syntax for quotes, but I know we
have one.)

> +    error ("field %qD has Java class type", decl);

C++ doesn't have fields; it has non-static data members.  I suggest:

  non-static data member %qD has Java class type

> +	  error ("parameter %qD invalidly declared Java class type", decl);

Just:

  parameter %qD has Java class type

The parameter may be invalid; the class type is not.

> +	error ("returning Java type %q#T by value", return_type);

  return type has Java class type %q#T

> +  else if (TYPE_FOR_JAVA (elt_type))
> +    {
> +      error ("Java class %q#T object allocated using placement new", elt_type);
> +      return error_mark_node;
> +    }

I don't think this one is safe.  We can't know that placement new
doesn't do heap allocation.

-- 
Mark Mitchell
CodeSourcery
mark@codesourcery.com
(650) 331-3385 x713


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]