Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

typedispatcher #1373

Open
wants to merge 20 commits into
base: master
from

Conversation

3 participants
@tom-wa
Copy link
Contributor

commented Feb 24, 2017

Type system preview only

currently code is a mess - will be refactored and cleaned later

Purpose

demo: scoping, sub types and recursive (depth-first) type hierarchy traversing, subtype checking

depends on #1321, #1334 (and some ugly hack in types)

@markus2330

  • any preferences on how to handle cyclic type dependencies ?
  • would you be ok with having a max-depth for type hierarchy levels for now ?
  • mapping metadata to plugins: i guess the easiest way would be parsing METADATA.ini during the build process and create a static struct. any other suggestions ?
  • i'd say abbreviations should be handled by the validation plugins like
          int validateSimple(key, configString, errorKey)
           
           type = enum('a', 'b', 'c')
               => enum::validateSimple(key, "'a', 'b', 'c'", errorKey)
           type = date("ISO|datetime complete")
               => date::validateSimple(key, "ISO|datetime complete", errorKey)
    
    what do you think ?

Checklist

  • commit messages are fine (with references to issues)
  • I ran all tests and everything went fine
  • I added unit tests
  • affected documentation is fixed
  • I added code comments, logging, and assertions
  • meta data is updated (e.g. README.md of plugins)
@markus2330

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

Please write a README.md so that we know what you tried to achieve. Please start with very simple examples. Please remember that the log output makes only sense to you.

Why not use Elektra's logger, are there any issues with it?

any preferences on how to handle cyclic type dependencies ?

Please abort with an error unless we find a real use case where we need it.

would you be ok with having a max-depth for type hierarchy levels for now ?

It is a bit weird, especially because you use dynamic memory allocations anyway.

mapping metadata to plugins: i guess the easiest way would be parsing METADATA.ini
during the build process and create a static struct. any other suggestions ?

I created the issue #1374 for discussions.

i'd say abbreviations should be handled by the validation plugins like

What are abbreviations?

@tom-wa

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2017

Why not use Elektra's logger, are there any issues with it?

vim folding for #if , i initially had addition code within them. will look into to logger again

Please abort with an error unless we find a real use case where we need it.

i was more worried about the costs of checking for cycles, thats why i also asked about a max-depth

It is a bit weird, especially because you use dynamic memory allocations anyway.

the type traversal is recursive, thats why i thought setting a max depth might be a good idea

What are abbreviations?

the examples below, using type = long or type = enum (... instead of adding multiple check/X metakeys

should i also implement a undef for types ?

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

the examples below

The example was the part that was unclear. What does it show? A function declaration and output from printf?

should i also implement a undef for types ?

I do not see a necessity for it. Better keep the features as is (there are enough already?) and let us add something if it is really needed.

@tom-wa

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2017

@markus2330
the typedispatcher has 2 jobs: read and build a "type tree" with all defined types, and validate the keys, but i'm currently stuck at the decision when the "type tree" should be build and updated.
there are several options:

  • only read it once and the beginning of kdbGet and never update it,
  • read it every time the plugin is called and overwrite the whole tree,
  • or read it once at kdbGet and if a key has a sync flag check for type data and update the type tree if needed

the first 2 options would be the easiest to implement, but the last probably the most useful ?

@tom-wa tom-wa force-pushed the tom-wa:type branch 7 times, most recently from ef6b935 to 75fd48c Feb 28, 2017

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2017

Sorry, I did not see this earlier. As discussed: always discard and rebuild.

@tom-wa tom-wa force-pushed the tom-wa:type branch from 75fd48c to 3377863 Mar 5, 2017

@tom-wa tom-wa changed the title typedispatcher: create and traverse typetree typedispatcher Mar 5, 2017

@tom-wa tom-wa force-pushed the tom-wa:type branch from 1aad518 to 35739c7 Mar 6, 2017

@tom-wa

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2017

@markus2330 i think we have to split usedby/plugin into checkby and usedby in METADATA.ini.
check/type is currently used by 2 plugins: type and range but i can't distinguish if type or range should validate check/type
and i need a way to figure out which plugin needs which metadata

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2017

i think we have to split usedby/plugin into checkby and usedby in METADATA.ini.

Yes, good idea!

@tom-wa tom-wa force-pushed the tom-wa:type branch from 9ab6bc0 to e2582c9 May 8, 2017

@tom-wa

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2017

using check/type instead of check/range/type currently wont work because typedispatcher only checks keys below check/range to create it's internal datatypes so i changed it back until i find a proper solution.

@markus2330 markus2330 requested a review from e1528532 May 8, 2017

@markus2330

This comment has been minimized.

Copy link
Contributor

commented May 8, 2017

Hi! Is it nevertheless ready for review? @e1528532 Can you take a look?

@tom-wa

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2017

yes. it depends on #1321

@markus2330

This comment has been minimized.

Copy link
Contributor

commented May 8, 2017

Okay, I merged #1321.

@e1528532 do you have time to take a look?

@tom-wa tom-wa force-pushed the tom-wa:type branch from e2582c9 to ae90536 May 9, 2017

@markus2330

This comment has been minimized.

Copy link
Contributor

commented May 16, 2017

Did you use a different version than 3.8 of llvm for reformat?

@tom-wa

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2017

Did you use a different version than 3.8 of llvm for reformat?

i formated testmod_typedispatcher by hand because with clang-format it becomes unreadable.
i'll reformat it before it gets merged

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2017

@e1528532 could you please take a look at this PR?

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2017

@tom-wa Could you please document how the scoping (shadowing of define/type) works? Then @domhof could implement it in the same way in the code generator.

@e1528532

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2017

@tom-wa i have a small issue with the test cases. Judging from the log the typedispatcher seems to do its thing, but then ultimately it tells me it can't load the range or enum plugin. For instance the output for test_simple:

60: Getting type definitions from user/tests/typedispatcher
60:      Key define/type/ace is 1 levels below define/type
60:      Key define/type/ace/check/range is 3 levels below define/type
60:      Key define/type/bar is 1 levels below define/type
60:      Key define/type/bar/check/range is 3 levels below define/type
60:      Key define/type/foo is 1 levels below define/type
60:      Key define/type/foo/check/range is 3 levels below define/type
60:      Key define/type/ace is 1 levels below define/type
60:      Key define/type/bar is 1 levels below define/type
60:      Key define/type/foo is 1 levels below define/type
60:     defines type ace
60:     defines type bar
60:     defines type foo
60: populating ace
60: datatype ace found in config->types
60:      Key define/type/ace/check/range is 1 levels below define/type/ace/check
60:     define/type/ace/check/range:(0-4,6-9)
60:             adding Metadata check/range:(0-4,6-9) to key range
60: populating bar
60: datatype bar found in config->types
60:      Key define/type/bar/check/range is 1 levels below define/type/bar/check
60:     define/type/bar/check/range:(3-5,8-20)
60:             adding Metadata check/range:(3-5,8-20) to key range
60: populating foo
60: datatype foo found in config->types
60:      Key define/type/foo/check/range is 1 levels below define/type/foo/check
60:     define/type/foo/check/range:(4-20)
60:             adding Metadata check/range:(4-20) to key range
60: Getting type definitions from user/tests/typedispatcher/all
60: validating user/tests/typedispatcher/all:(4)
60:      Key type/#0 is 1 levels below type
60:      Key type/#1 is 1 levels below type
60:      Key type/#2 is 1 levels below type
60:     with type ace
60:     parseTypeString ace returned 0x0, typeName: ace
60:             user/tests/typedispatcher/all has SUBTYPE
60:     range-validation
60: replacing parameters in 0-4,6-9
60: result: 0-4,6-9
60:             check/range:(0-4,6-9)
60: plugin providing range not loaded
60: ERROR: failed to load plugin providing range
60:                     checkKey(config, user/tests/typedispatcher/all:(4), range) returned -1
60: Key user/tests/typedispatcher/all failed to validate

The range and the enum plugin are both compiled though i think, and running the enum and the range test both succeed. Anything else to keep in mind?

@e1528532
Copy link
Contributor

left a comment

i think the code looks quite good in general, i've mostly added some style related comments which you may follow or not.
I've only seen one potential mistake on a first glance (the strncpy). ASAN and static code checkers are also happy with it.
I give you more feedback on the actual type problem once we resolved my testcase issue, so i can look how things correspond to my understanding of how the type system is supposed to work by checking the tests and maybe even adding some more.

typechecker.c
pluginhelper.c
LINK_ELEKTRA
elektra-proposal

This comment has been minimized.

Copy link
@e1528532

e1528532 Jun 22, 2017

Contributor

please also add elektra-kdb here otherwise it doesnt compile on clang

ssize_t newLen = elektraStrLen (preparedString) + sizeof (tmp) + 2;
elektraRealloc ((void **)&preparedString, newLen);
strcat (preparedString, "%");
strncat (preparedString, tmp, sizeof (tmp));

This comment has been minimized.

Copy link
@e1528532

e1528532 Jun 22, 2017

Contributor

The compiler gives me this warning. shouldn't be strlen be used instead of sizeof here?
helpers.c:274:37: warning: size argument in 'strncat'
call appears to be size of the source [-Wstrncat-size]
strncat (preparedString, tmp, sizeof (tmp));

This comment has been minimized.

Copy link
@tom-wa

tom-wa Jul 16, 2017

Author Contributor

what compiler are you using ? neither gcc 6.3 locally nor the buildserver are showing any warnings.
it's valid but by far not the best solution

// initialize plugin configuration
DispatchConfig * initDispatchConfig ()
{
DispatchConfig * config = NULL;

This comment has been minimized.

Copy link
@e1528532

e1528532 Jun 22, 2017

Contributor

could be directly initialized with elektraCalloc, reported by cppcheck. there are some more cases like this (just run cppcheck yourself if you wanna see them). But as its in the end only a style question, its up to you how you prefer it.

@@ -0,0 +1,99 @@
#include <kdbplugin.h>

This comment has been minimized.

Copy link
@e1528532

e1528532 Jun 22, 2017

Contributor

Wouldn't you also have something like

#ifndef ELEKTRA_PLUGIN_TYPEHELPER_H
#define ELEKTRA_PLUGIN_TYPEHELPER_H

here to prevent possible issues? also the file header is missing

if (!typeKey) return ERROR;

const char * typeName = keyBaseName (typeKey);
#ifdef DEVBUILD

This comment has been minimized.

Copy link
@e1528532

e1528532 Jun 22, 2017

Contributor

Wouldn't it be better to use the ELEKTRA_LOG functionality, so its not required to handle the logging of this plugin separately via this DEVBUILD flag?
you also have a VERBOSEBUILD flag, but i think for this different log levels could be utilized
this basically holds for all such cases, therefore it'd not specifially mark each occasion again.

This comment has been minimized.

Copy link
@tom-wa

tom-wa Jul 16, 2017

Author Contributor

i'll drop both of them when i'm done, they are just for internal debugging (+ vim hides/folds them nicely)

#include <strings.h>


// hekoer fir freeTyoes

This comment has been minimized.

Copy link
@e1528532

e1528532 Jun 22, 2017

Contributor

looks like you have a little typo there

#include <kdbease.h> //elektraArrayIncName
#include <kdbmodule.h> //elektraModulesInit, elektraModulesClose
#include <kdbos.h> //elektraNamespace;
#include <kdbprivate.h> //elektraPluginClose. elektraPluginOpen, elektraStrNDup

This comment has been minimized.

Copy link
@e1528532

e1528532 Jun 22, 2017

Contributor

ok thats really a minor thing but maybe consistent about using , ; and . or nothing at all, otherwise one could maybe believe that there is some meaning behind it

elektraNamespace scopeNS = keyGetNamespace (scope);
if (scopeNS == KEY_NS_SPEC)
{
if (keyRel2 (scope, key, ELEKTRA_REL_BELOW_IGNORE_NS) <= 0)

This comment has been minimized.

Copy link
@e1528532

e1528532 Jun 22, 2017

Contributor

in other parts of the code you don't use braces for single line return statements though cause of the nested structure here it might be appropriate. Maybe even simplify this as
return keyRel2 (scope, key, ELEKTRA_REL_BELOW_IGNORE_NS) > 0; ?

Key * getTypeKey (DispatchConfig * config, const char * type)
{
Key * lookup = ksLookupByName (config->types, type, KDB_O_NONE);
return lookup;

This comment has been minimized.

Copy link
@e1528532

e1528532 Jun 22, 2017

Contributor

i prefer directly returning the method's result in such cases

const char * ptr = typeString;
while (*ptr)
{
if (*ptr == '(' || *ptr == ' ') break;

This comment has been minimized.

Copy link
@e1528532

e1528532 Jun 22, 2017

Contributor

directly below in line 326 you have such check in your loop condition, which i also prefer.

@tom-wa

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2017

@e1528532

it tells me it can't load the range or enum plugin

is it the same for the type-plugin ?

@tom-wa

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2017

is it necessary to set an error if we run out of memory or would returning with -1 or calling exit be enough ? i mean, it realloc/calloc actually fails there would be a high chance that setting the error message will fail too

@tom-wa tom-wa force-pushed the tom-wa:type branch from 25b5fe0 to 22ee9aa Jul 16, 2017

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

is it necessary to set an error if we run out of memory or would returning with -1 or calling exit be enough ? i mean, it realloc/calloc actually fails there would be a high chance that setting the error message will fail too

return -1 is okay for now, it is done at many places. If you have the parentKey, please set the error, it will save time if we support systems where malloc fails under normal circumstances. I like @sanssecours suggestion to have a macro explicitly for this case, then we need to implement the special case for "we need an error message without allocating anything" only there. But its not a goal now, testing out-of-memory situations is a beyond 1.0 goal.

@markus2330 markus2330 added this to to implement in lcdproc Aug 2, 2017

@markus2330 markus2330 moved this from to implement to low priority in lcdproc Apr 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.