[PATCH]: PR29066 ptrmemfunc_vbit_in_delta is broken

Roger Sayle roger@eyesopen.com
Fri Nov 3 03:50:00 GMT 2006

Hi Ryan,

On Thu, 2 Nov 2006, Ryan Mansfield wrote:
> Any feedback is appreciated.

I'm not a C++ front-end person, so I can't comment on the technical
aspects, but there are a number of coding style issues that need to
be addressed (if this change is appropriate).

+ if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_delta) {

In the GNU coding style, the open "{" should appear on the next line
indented two spaces in from the start of the "if", and the body of
the if-then-else indented two spaces futher still.

For long expressions like the one above it may be necessary to wrap
the condition to avoid having lines longer than about 78 characters.
Hence something like:

+     == ptrmemfunc_vbit_in_delta)
+   {
+     ...
+   }

In your patch, you introduce a number of new temporary variables;
e, e1, e2, pfn0 and delta0.  As these are not used outside of the
scope of the new if you're adding, you can/should define these
variables in that scope.  And if line wrapping isn't a complication
you can define these variables on the same line on which they are
initialized if the dependencies permit.

+	  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_delta)
+           {
+	      tree pfn0 = pfn_from_ptrmemfunc (op0);
+	      tree delta0 = build_ptrmemfunc_access_expr (op0,
+						          delta_identifier);
+             tree e1 = ...;

At one point you introduce two blank lines into the middle of a
function.  Normally vertical whitepace should be limited to one
blank line inside functions, but two is/are reasonable as a
separator at global scope.

I hope this helps.


More information about the Gcc-patches mailing list