Additional Type Information #1236

markuspf · 2017-04-12T22:55:48Z

This pull request does 2 things

It adds functions IsCategory, FilterByName, IdOfFilter, IdOfFilterByName, and CategoryByName.
It stores the location of the declaration of filters, operations, attributes, and properties in global lists.

markuspf · 2017-04-12T22:56:17Z

Actually this is missing a function to query the declaration location. I'll add this.

markuspf · 2017-04-12T23:24:16Z

The LocationOfDeclaration will need some more prodding, and probably I can improve some of the structures used for search.

codecov · 2017-05-08T10:59:36Z

Codecov Report

Merging #1236 into master will decrease coverage by 2.26%.
The diff coverage is 92.24%.

@@            Coverage Diff             @@
##           master    #1236      +/-   ##
==========================================
- Coverage   63.98%   61.71%   -2.27%     
==========================================
  Files         995      911      -84     
  Lines      325334   267762   -57572     
  Branches    13032    13036       +4     
==========================================
- Hits       208155   165254   -42901     
+ Misses     114381    99825   -14556     
+ Partials     2798     2683     -115

Impacted Files	Coverage Δ
src/c_oper1.c	`88.43% <100%> (+0.19%)`	⬆️
lib/read1.g	`100% <100%> (ø)`
lib/global.gi	`36.22% <100%> (+0.5%)`	⬆️
lib/type.g	`66.39% <100%> (+1.13%)`	⬆️
lib/coll.gd	`90.66% <100%> (+0.25%)`	⬆️
lib/function.g	`52.43% <100%> (-0.58%)`	⬇️
lib/type.gi	`37.25% <72%> (+33.4%)`	⬆️
lib/filter.g	`88.15% <83.33%> (-1.5%)`	⬇️
lib/oper.g	`75.46% <96.87%> (-0.05%)`	⬇️
extern/gmp/mpn/generic/toom3_sqr.c	`0% <0%> (-100%)`	⬇️
... and 242 more

* IsCategory, IsProperty, IsAttribute determine wheter an object is a category, property, or attribute respectively * FilterByName retrieve filter by name * IdOfFilter return the ID of a given filter * IdOfFilterByName return the ID if a filter given by its name * CategoryByName return category by name

Functionality to store the location of the declaration of * filters * operations * attributes * properties

markuspf · 2017-08-18T08:51:39Z

I rebased this and tweaked it a tiny bit. It would be good to get some feedback; I'll probably discover more things I would like to be able to do with objects and types later.

markuspf · 2017-08-18T09:22:32Z

I'll fix the HPC-GAP failures.

fingolfin · 2017-08-18T10:21:08Z

lib/type.gi

+        fi;
+    fi;
+
+    fid := PositionProperty(BIND_LOCS, x -> x[1] = NAME_FUNC(object));


I wonder if we really need all that: our filters and operation are T_FUNCTION objects internally, and thus FILENAME_FUNC and LOCATION_FUNC (resp. STARTLINE_FUNC / ENDLINE_FUNC) are applicable for them. These slots are currently never set for filters & operations, as far as I can tell...

So can't we just add SET_* counterparts for these to the kernel interface, and store the information in there?

Perhaps I am missing something?

fingolfin · 2017-08-18T10:25:14Z

tst/testinstall/type.tst

+gap> IsProperty(IsFinite);
+true
+gap> IsOperation(IsFinite);
+true


Perhaps do this instead (which I find a bit easier to read, esp. as I don't need to scroll around as much; and testing more things later one becomes easier?)

gap> test := x -> List([IsFilter, IsCategory, IsAttribute, IsProperty, IsOperation], f -> f(x));; gap> test(IsFinite) [ true, false, true, true, true ] ...

fingolfin · 2017-08-18T10:25:54Z

tst/testinstall/type.tst

+gap> IsProperty(Size);
+false
+gap> IsOperation(Size);
+true


So... should there also be negative tests, i.e. for what happens if one applies IsFilter to, say, a boolean, or a string, or ... ?

fingolfin · 2017-08-18T10:29:13Z

lib/filter.g

+##
+##  <Description>
+##  finds the id of the filter, or the filter with name <A>name</A> respectively,
+##  in the global FILTERS list. 


I think it would be good to explain here why FilterByName is useful -- at first glance, x := FilterByName("IsFinite"); is just a longer way of saying x := IsFinite. But of course it's possible to have filters which are not bound to a global variable with matching names. So perhaps state just that: That a filter has an intrinsic name, and there need not be a global variable with matching name; and even if there is such a gvar, it need not be bound to that filter.

Also perhaps have a "see also" ref pointing to CategoryByName?

Perhaps another reason would be that a filter can be named Tester(FOO), which is not a valid gvar identifier. Of course we now changed that particular example, but the general problem remains (and is solved by FilterByName, I guess)

fingolfin · 2017-08-18T10:31:04Z

lib/filter.g

+##  <Func Name="FilterByName" Arg="name"/>
+##
+##  <Description>
+##  finds the id of the filter, or the filter with name <A>name</A> respectively,


My first thought was "what is the id of a filter"? I only later saw IdOfFilter below. I'd therefore recommend splitting the documentation of IdOfFilterByName and FilterByName. The former might start like this:
"find the id of the given filter, as defined in IdOfFilter.

fingolfin · 2017-08-18T10:46:22Z

lib/function.g

-            flags := [];
-        fi;
+        flafs := FLAGS_FILTER(op);
+        if flafs <> false then


Missing indentation? Perhaps you did this so as to not obscure the review with pointless whitespace diffs?

fingolfin · 2017-08-18T10:47:34Z

lib/function.g

-        fi;
+        flafs := FLAGS_FILTER(op);
+        if flafs <> false then
+        flags := TRUES_FLAGS(FLAGS_FILTER(op));


Perhaps you meant to use flafs here, too? Otherwise you could just as well get rid of it again.

fingolfin · 2017-08-18T10:50:26Z

lib/filter.g

+        fi;
+    od;
+    return fail;
+end);


So this function is essentially equivalent to name -> PositionProperty(FILTERS, f -> NAME_FUNC(f) = name) ? Of course PositionProperty is not available at the time this file is read. But it makes me wonder how useful this function really is... seem to me mostly something for core developers? Perhaps you are concerned about this helper being available very early on? Because if not, then I'd consider going with the much shorter definition, and simply defining this in a new filter lib/filter.gi

fingolfin · 2017-08-18T10:50:49Z

lib/filter.g

+    else
+        return fail;
+    fi;
+end);


Similarly, this is name -> First(FILTERS, f -> NAME_FUNC(f) = name);

fingolfin · 2017-08-18T10:57:01Z

lib/oper1.g

+                narg,
+                flags,
+                rank,
+                rec( line := INPUT_LINENUMBER(), file := INPUT_FILENAME() ) ] );


So what does this add compared to FILENAME_FUNC(method)? The location the method is installed? So that would differ for the few instances were a function is defined in one place, and then installed in another? That seems somewhat rare... But in any case: If I guess right, then this should be explicitly documented, instead of leaving the reader to wonder.

Also, a test case showing the difference would be nice?

fingolfin · 2017-08-23T17:38:55Z

This needs to be rebased.

markuspf · 2017-08-23T18:37:59Z

I think actually this is now obsolete: The helper functions have been added in #1593, and the location information should be stored in the function object; this just requires a little more work (which we discussed elsewhere).

fingolfin · 2017-08-31T08:45:46Z

So if this is obsolete, shall we close it?

markuspf added do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements labels Apr 12, 2017

markuspf force-pushed the type-information-2 branch 2 times, most recently from e39b3d4 to 6946543 Compare April 12, 2017 23:23

markuspf force-pushed the type-information-2 branch from 6946543 to 010fb3f Compare May 8, 2017 10:58

markuspf added 2 commits August 18, 2017 09:49

Store location info for declarations

865d99d

Functionality to store the location of the declaration of * filters * operations * attributes * properties

markuspf force-pushed the type-information-2 branch from 010fb3f to 865d99d Compare August 18, 2017 08:50

markuspf removed the do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) label Aug 18, 2017

markuspf requested a review from fingolfin August 18, 2017 08:50

fingolfin reviewed Aug 18, 2017

View reviewed changes

markuspf mentioned this pull request Aug 18, 2017

Add Helpers for Information about Operations and Filters #1593

Merged

fingolfin closed this Aug 31, 2017

fingolfin reopened this Aug 31, 2017

markuspf closed this Sep 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional Type Information #1236

Additional Type Information #1236

markuspf commented Apr 12, 2017

markuspf commented Apr 12, 2017

markuspf commented Apr 12, 2017

codecov bot commented May 8, 2017 •

edited

Loading

markuspf commented Aug 18, 2017

markuspf commented Aug 18, 2017

fingolfin Aug 18, 2017

fingolfin Aug 18, 2017

fingolfin Aug 18, 2017

fingolfin Aug 18, 2017

fingolfin Aug 18, 2017

fingolfin Aug 18, 2017

fingolfin Aug 18, 2017

fingolfin Aug 18, 2017

fingolfin Aug 18, 2017

fingolfin Aug 18, 2017

fingolfin Aug 18, 2017

fingolfin Aug 18, 2017

fingolfin commented Aug 23, 2017

markuspf commented Aug 23, 2017

fingolfin commented Aug 31, 2017

Additional Type Information #1236

Additional Type Information #1236

Conversation

markuspf commented Apr 12, 2017

markuspf commented Apr 12, 2017

markuspf commented Apr 12, 2017

codecov bot commented May 8, 2017 • edited Loading

Codecov Report

markuspf commented Aug 18, 2017

markuspf commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Aug 23, 2017

markuspf commented Aug 23, 2017

fingolfin commented Aug 31, 2017

codecov bot commented May 8, 2017 •

edited

Loading