Assertion Support Review

By Mathias Panzenböck <panzi@complang.tuwien.ac.at>

Only the parts of the listed files that where changed in order to enable assertion support where reviewed.

Reviewed files:

configure.ac

Reviewed version.

m4/assertion.m4

Reviewed version.

src/lib/gnu/java/lang/VMClassLoader.java

Reviewed version.

For each Map the generic parameters are missing. I think Map<String,Boolean> would be appropriate.

 372   static final Map packageAssertionMap = 
 373     Collections.unmodifiableMap(packageAssertionStatus0(Boolean.TRUE, Boolean.FALSE));
 374   
 375   static native final Map packageAssertionStatus0(Boolean jtrue, Boolean jfalse);
 376   /**
 377    * The system default for package assertion status. This is used for all
 378    * ClassLoader's packageAssertionStatus defaults. It must be a map of
 379    * package names to Boolean.TRUE or Boolean.FALSE, with the unnamed package
 380    * represented as a null key.
 381    *
 382    * @return a (read-only) map for the default packageAssertionStatus
 383    */
 384    
 385   static final Map packageAssertionStatus() {
 386     return packageAssertionMap;
 387   }
 388 
 389   static final Map classAssertionMap = 
 390     Collections.unmodifiableMap(classAssertionStatus0(Boolean.TRUE, Boolean.FALSE));
 391   
 392   static native final Map classAssertionStatus0(Boolean jtrue, Boolean jfalse);
 393 
 394   /**
 395    * The system default for class assertion status. This is used for all
 396    * ClassLoader's classAssertionStatus defaults. It must be a map of
 397    * class names to Boolean.TRUE or Boolean.FALSE
 398    *
 399    * @return a (read-only) map for the default classAssertionStatus
 400    */
 401   static final Map classAssertionStatus() {
 402     return classAssertionMap;
 403   }

src/native/include/Makefile.am

Reviewed version.

src/native/jni.h

Reviewed version.

src/native/vm/gnu/java_lang_VMClassLoader.c

Reviewed version.

The functions Java_java_lang_VMClassLoader_packageAssertionStatus0 and Java_java_lang_VMClassLoader_classAssertionStatus0 are almost identical (redundancy!). The only difference is:

 489                 if (item->package == false) {

Vs.

 562                 if (item->package == true) {

This functions should be unified.

src/native/vm/sun/jvm.c

Reviewed version.

src/vm/Makefile.am

Reviewed version.

src/vm/assertion.c

Reviewed version.

Description of the parameters and the behaviour (depending on the values of the parameters) of the function assertion_ea_da is missing.

  50 /* assertion_ea_da *************************************************************
  51 
  52    Handle -ea:/-enableassertions: and -da:/-disableassertions: options.
  53 
  54 *******************************************************************************/
  55 
  56 void assertion_ea_da(const char *name, bool enabled) {
  57         bool              package;
  58         size_t            len;
  59         char             *buf;
  60         assertion_name_t *item;
  61         int32_t           i;
  62 
  63         if (name == NULL) {
  64                 assertion_user_enabled = enabled;
  65                 return;
  66         }
  67 
  68         package = false;
  69         len     = strlen(name);
  70 

What is the logic behind those string operations? A short comment like "converting dot separated class name representation to slash separated representation" would be good. Maybe put this into a helper function?

  71         if (name[len - 1] == '/') {
  72                 return;
  73         }
  74 
  75         buf = strdup(name);
  76         if (buf == NULL) {
  77                 vm_abort("assertion_ea_da: strdup failed: %s", strerror(errno));
  78         }
  79 
  80         if ((len > 2) && (strcmp(name + (len - 3), "...") == 0)) {
  81                 package = true;
  82                 assertion_package_count += 1;
  83 #if defined(WITH_CLASSPATH_SUN)
  84                 buf[len - 2] = '\0';
  85                 buf[len - 3] = '/';
  86 #else
  87                 buf[len - 3] = '\0';
  88 #endif
  89         }
  90         else {
  91                 assertion_class_count += 1;
  92         }
  93 
  94         len = strlen(buf);
  95         for (i = 0; i < len; i++) {
  96 #if defined(WITH_CLASSPATH_SUN)
  97                 if (buf[i] == '.') {
  98                         buf[i] = '/';
  99                 }
 100 #else
 101                 if (buf[i] == '/') {
 102                         buf[i] = '.';
 103                 }
 104 #endif
 105         }
 106 
 107         item          = NEW(assertion_name_t);
 108         item->name    = buf;
 109         item->enabled = enabled;
 110         item->package = package;
 111 
 112         if (list_assertion_names == NULL) {
 113                 list_assertion_names = list_create(OFFSET(assertion_name_t, linkage));
 114         }
 115         list_add_last(list_assertion_names, item);
 116 }

src/vm/assertion.h

Reviewed version.

src/vm/vm.c

Reviewed version.

src/vmcore/class.c

Reviewed version.

src/vmcore/class.h

Reviewed version.

src/vmcore/linker.c

Reviewed version.

src/vmcore/loader.c

Reviewed version.

tests/regression/assertion/Makefile.am

Reviewed version.

tests/regression/assertion/Test.sh

Reviewed version.

tests/regression/assertion/testassertions.java

Reviewed version.

tests/regression/assertion/packagetest/testassertions.java

Reviewed version.

cacaowiki: assertions/review (last edited 2008-02-06 19:40:51 by panzi)