On Fri, Feb 27, 2015 at 5:36 PM, Micah Fedke <micah.fe...@collabora.co.uk> wrote: > +def _gen_tolerance(name, rettype, args): > + """Return the tolerance that should be allowed for a function for the > + test vector passed in. Return -1 for any vectors that would push the > + tolerance outside of acceptable bounds
It seems to alternatively return scalars or lists. Why? > + """ > + if name in simple_fns: > + return simple_fns[name] > + elif name in complex_fns: > + if name in componentwise: > + # for componentwise functions, call the reference function > + # for each component (scalar components multiplex here) > + # eg. for fn(float a, vec2 b, vec2 c) call: > + # _fn_ref(a, b[0], c[0]) > + # _fn_ref(a, b[1], c[1]) > + # and then capture the results in a list > + errors = [] > + badlands = [] > + component_tolerances = [] > + for component in range(rettype.num_cols * rettype.num_rows): > + current_args = [] > + for arg in args: > + # sanitize args so that all analyze operations can be > performed on a list > + sanitized_arg = _listify(arg) > + current_args.append(sanitized_arg[component % > len(sanitized_arg)]) This is confusing. Does this ever happen on matrix "outputs"? When would component be > len(sanitized_arg)? And when it is, why does modding it by the length again make sense? > + cur_errors, cur_badlands, cur_component_tolerances = > _analyze_ref_fn(complex_fns[name], current_args) > + errors.extend(cur_errors) > + badlands.extend(cur_badlands) > + component_tolerances.extend(cur_component_tolerances) > + else: > + # for non-componentwise functions, all args must be passed in in > a single run > + # sanitize args so that all analyze operations can be performed > on a list Why did you choose to split these up? Why not just make the various functions just always take a list and operate on the whole list? It seems like that would save a lot of heartache and confusion... > + current_args = map(_listify, args) > + errors, badlands, component_tolerances = > _analyze_ref_fn(complex_fns[name], current_args) > + # results are in a list, and may have one or more members > + return -1.0 if True in badlands else map(float, component_tolerances) If badlands is just a true/false (as it seems to be), makes sense to not keep it as a list and just have it as a plain bool right? Also the a if b else c syntax is generally meant for "a is the super-common case, but in these awkward situations, it may be c". Or "I _really_ _really_ want a single expression here". It doesn't seem like it'd reduce readability to just do the more common if True in badlands: return -1 else: return map(float, component_tolerances) Which also makes it more obvious that something funky is going on since one path returns a scalar while the other returns a list. > + elif name in mult_fns: > + x_type = glsl_type_of(args[0]) > + y_type = glsl_type_of(args[1]) > + # if matrix types are involved, the multiplication will be matrix > multiplication > + # and we will have to pass the args through a reference function > + if x_type.is_vector and y_type.is_matrix: > + mult_func = _vec_times_mat_ref > + elif x_type.is_matrix and y_type.is_vector: > + mult_func = _mat_times_vec_ref > + elif x_type.is_matrix and y_type.is_matrix: > + mult_func = _mat_times_mat_ref > + else: > + # float*float, float*vec, vec*float or vec*vec are all done as > + # normal multiplication and share a single tolerance > + return mult_fns[name] > + # sanitize args so that all analyze operations can be performed on a > list > + errors, badlands, component_tolerances = _analyze_ref_fn(mult_func, > _listify(args)) > + # results are in a list, and may have one or more members > + return -1.0 if True in badlands else map(float, component_tolerances) > + else: > + # default for any operations without specified tolerances > + return 0.0 We've had a few rounds of these reviews, and I'm still generally confused by the specifics of how this is done. I normally don't have this much trouble reading code... not sure what's going on. Feel free to find someone else to review if you're getting frustrated by my apparently irreparable state of confusion. Cheers, -ilia _______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit
