Module(): analyse reference class once and reuse C++Class objects

[mdsumner]

Two changes in R/Module.R, inside Module()

#1514

speeds up load times, relevant especially to terra and gdalraster but affects other modules packages https://github.com/mdsumner/Rcpp/blob/code-docs-speedup-namespace-load/rcppmoduleloadtime.md

[Enchufa2]

Edit the ChangeLog too, please.

[kevinushey]

Thanks, the core change looks right to me. I compared the initialize refMethodDef produced by setRefClass(methods = list(initialize = ...)) with the one produced by the old generator$methods(initialize = ...) path and they are identical (same mayCall, same superClassMethod), and Module::classes_info() / Module::get_class() build CppClass the same way on the C++ side, so reusing the objects from Module__classes_info is safe.

A few things I noticed in the loops this PR touches. The first is a direct follow-on to the change; the other two are pre-existing but cheap to fix in the same pass, and the second one works against the load-time goal here. Happy to see them deferred to a follow-up if you prefer.

1. methods::getClass(clname) ignores where (R/Module.R line 246)

The lookup goes through the global class table rather than where. If two loaded packages expose a C++ class with the same name (both become Rcpp_Foo), the second Module() call can pick up the first package's definition and write this module's .pointer / .module into the wrong fieldPrototypes. The freshly created definition is already on the generator, so this is also one fewer lookup:

        classDef <- generator$def

(I've attached this as an inline suggestion as well.)

2. Converter registration runs once per module function (R/Module.R lines 305-323)

The grep(converter_rx, functions) / setAs() block sits inside for (fun in functions), so with N functions and M converters it does N regex scans and N*M setAs() calls, and the inner loop clobbers the outer fun. Since the converter body captures storage[[fun]] via substitute(), the block just needs to run after storage is fully populated:

    # functions
    functions <- .Call( Module__functions_names, xp )
    for( fun in functions ){
        storage[[ fun ]] <- .get_Module_function( module, fun, xp )
    }

    # register as(FROM, TO) methods
    converter_rx <- "^[.]___converter___(.*)___(.*)$"
    for( fun in grep( converter_rx, functions, value = TRUE ) ){             # #nocov start
        from <- sub( converter_rx, "\\1", fun )
        to   <- sub( converter_rx, "\\2", fun )
        converter <- function( from ){}
        body( converter ) <- substitute( { CONVERT(from) },
            list( CONVERT = storage[[fun]] )
        )
        setAs( from, to, converter, where = where )
    }                                                                       # #nocov end

3. grepl("show", ...) is a substring match (R/Module.R line 273)

A class that exposes e.g. showInfo but no show method gets an S4 show method that calls object$show(), so auto-printing an instance errors with "show is not a valid field or method name for reference class". An exact match avoids that:

        if( "show" %in% names(CLASS@methods) ){
            setMethod( "show", clname, function(object) object$show(), where = where )  # #nocov
        }

4. ChangeLog / DESCRIPTION

Seconding @Enchufa2: a ChangeLog entry and the usual DESCRIPTION Version/Date micro-bump, please. One small thing worth a line there: generator$methods() used to call utils::globalVariables("initialize", ...) on the defining namespace as a side effect, and setRefClass(methods = ) does not. That only matters for a package whose own R code references initialize as a free symbol, which would now pick up a "no visible binding" NOTE from codetools, so I don't think it needs a code change, just a mention.

Also now that the second class loop no longer calls it, .get_Module_Class() has no callers and can be dropped (along with Module__get_class on the C++ side, or keep that one for ABI if you prefer).

[kevinushey]

The generator already carries the definition that setRefClass() just created, so this avoids the by-name lookup through the global class table (which can resolve to another package's Rcpp_<name> class when two packages expose the same C++ class name).

Suggested change

	classDef <- methods::getClass(clname)
	classDef <- generator$def