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

added builtin list::reverse for YCP list #3

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion VERSION
@@ -1 +1 @@
2.23.3
2.23.4
29 changes: 28 additions & 1 deletion libycp/src/YCPBuiltinList.cc
Expand Up @@ -1173,7 +1173,7 @@ l_swaplist (const YCPList &v, YCPInteger &i1, YCPInteger &i2){
/**
* @builtin list::swap
* @id list.swap
* @short Creates new list with swaped elemetns at offset i1 and i2.
* @short Creates new list with swaped elements at offset i1 and i2.
* @param list<flex1> v list
* @param integer i1 index of first element
* @param integer i2 index of second element
Expand Down Expand Up @@ -1218,6 +1218,32 @@ l_swaplist (const YCPList &v, YCPInteger &i1, YCPInteger &i2){
return ret;
}

static YCPValue
l_reverselist (const YCPList &v){
/**
* @builtin list::reverse
* @id list.reverse
* @short Creates new list with reversed order of elements.
* @param list<flex1> v list
* @return New list. Changed if offset is correct, otherwise return unchanged list
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the documentation right? I don't see such parameters

*
* @description
* Creates new list with reversed order of elements. Return nil if list is nil.
*
* @usage list::reverse ([0,1,2,3]) -> [3,2,1,0]
* @usage list::reverse ([]) -> []
*/
if (v.isNull ())
{
ycp2error ("Cannot reverse 'nil' list");
return YCPNull ();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it really problem? If so, then why continue with execution?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading documentation for you method I strongly don't see reason for such error log - Creates new list with reversed order of elements. Return nil if list is nil. so it is documented as common and expected behavior

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Wed, Jun 20, Josef Reidinger wrote:

  • \* @id list.reverse
    
  • \* @short Creates new list with reversed order of elements.
    
  • \* @param list<flex1> v list
    
  • \* @return New list. Changed if offset is correct, otherwise return unchanged list 
    
  • *
    
  • \* @description
    
  • \* Creates new list with reversed order of elements. Return nil if list is nil. 
    
  • *
    
  • \* @usage list::reverse ([0,1,2,3]) -> [3,2,1,0]
    
  • \* @usage list::reverse ([]) -> []
    
  • */
    
  • if (v.isNull ())
  • {
  • ycp2error ("Cannot reverse 'nil' list");
  • return YCPNull ();
  • }

After reading documentation for you method I strongly don't see reason for such error log - Creates new list with reversed order of elements. Return nil if list is nil. so it is documented as common and expected behavior

This is first time I add an ycp builtin.

I just copied code of another builtin and changed functionality
so I have no idea if this error log has some reason or not.

Tschuess,

Thomas Fehr

Thomas Fehr, SuSE Linux Products GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg)
Tel: +49-911-74053-0, Fax: +49-911-74053-482, Email: fehr@suse.de
GPG public key available.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depend case by case like for add new element to nil list it is reasonable to have it here.
From my POV reduce is similar case like e.g. sort ( just return list with different order ), so it make sense for me to not have here error message.
@mvidner Can you bring some light here?


YCPList ret = v;
ret->reverse();
return ret;
}

static YCPValue
l_tolist (const YCPValue &v)
{
Expand Down Expand Up @@ -1289,6 +1315,7 @@ YCPBuiltinList::YCPBuiltinList ()
{ "reduce", "flex1 (variable <flex1>, variable <flex1>, const list <flex1>, const block <flex1>)", (void *)l_reduce1, DECL_LOOP|DECL_SYMBOL|DECL_FLEX, ETCf },
{ "reduce", "flex1 (variable <flex1>, variable <flex2>, const flex1, const list <flex2>, const block <flex1>)", (void *)l_reduce2, DECL_LOOP|DECL_SYMBOL|DECL_FLEX, ETCf },
{ "swap", "list <flex> (const list <flex>, const integer, const integer)", (void *)l_swaplist, DECL_FLEX, ETCf },
{ "reverse", "list <flex> (const list <flex>)", (void *)l_reverselist, DECL_FLEX, ETCf },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this line is a little magic for me, but from list of DECL_FLEX it looks like it should also accept nil so DECL_NIL
@mvidner can you point me to some documentation to verify it? ( at least it is strange for me why only select have it here, but almost all function handle nil list )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DeclFlags are declared at

I cannot quite parse your question, but anyway, the problem is that we haven't really agreed on whether nil is an "exception" or a processable value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that definition of DeclFlags, but I don't understand it completelly especially part with DECL_NIL. It looks bogus for me. Some methods doesn't have it and still it looks like ti accept nil paramter.

{ NULL, NULL, NULL, ETC }
};

Expand Down
6 changes: 6 additions & 0 deletions libycp/src/YCPList.cc
Expand Up @@ -97,6 +97,12 @@ YCPListRep::remove (const int n)
}


void
YCPListRep::reverse()
{
std::reverse(elements.begin(), elements.end());
}

void
YCPListRep::swap (int x, int y)
{
Expand Down
7 changes: 7 additions & 0 deletions libycp/src/include/ycp/YCPList.h
Expand Up @@ -111,6 +111,12 @@ class YCPListRep : public YCPValueRep
*/
void remove(const int n);

/**
* Reverses the elements in the list. This function
* changes the list.
*/
void reverse();

/**
* Exchanges the elements at the indices x and y. This function
* changes the list.
Expand Down Expand Up @@ -248,6 +254,7 @@ class YCPList : public YCPValue
void push_back(const YCPValue& value) { ELEMENT->push_back(value); }
void set(const int n, const YCPValue& value) { ELEMENT->set (n, value); }
void remove(const int n) { ELEMENT->remove (n); }
void reverse() { ELEMENT->reverse(); }
void swap(int x, int y) { ELEMENT->swap (x, y); }
bool contains (const YCPValue& value) const { return CONST_ELEMENT->contains (value); }
void sortlist() { ELEMENT->sortlist (); }
Expand Down
17 changes: 17 additions & 0 deletions libycp/testsuite/tests/builtin/revlist.err
@@ -0,0 +1,17 @@
Parsed:
----------------------------------------------------------------------
list::reverse ([0, 1, 2, 3])
----------------------------------------------------------------------
Parsed:
----------------------------------------------------------------------
list::reverse ([0])
----------------------------------------------------------------------
Parsed:
----------------------------------------------------------------------
list::reverse ([])
----------------------------------------------------------------------
Parsed:
----------------------------------------------------------------------
list::reverse (nil)
----------------------------------------------------------------------
[Interpreter] tests/builtin/revlist.ycp:16 Argument (nil) to reverse(...) is nil
4 changes: 4 additions & 0 deletions libycp/testsuite/tests/builtin/revlist.out
@@ -0,0 +1,4 @@
([3, 2, 1, 0])
([0])
([])
(nil)
16 changes: 16 additions & 0 deletions libycp/testsuite/tests/builtin/revlist.ycp
@@ -0,0 +1,16 @@
# ---------------------------------------------------------
#
# Filename: revlist.ycp
#
# Purpose: test cases for YCP list::reverse builtin
#
# Creator: fehr@suse.de
#
# Maintainer: fehr@suse.de
#
# --------------------------------------------------------

(list::reverse([0,1,2,3]))
(list::reverse([0]))
(list::reverse([]))
(list::reverse(nil))
6 changes: 6 additions & 0 deletions package/yast2-core.changes
@@ -1,3 +1,9 @@
-------------------------------------------------------------------
Wed Jun 20 15:41:57 CEST 2012 - fehr@suse.de

- 2.23.4
- added builtin list::reverse for YCP list

-------------------------------------------------------------------
Tue Jun 19 07:42:53 UTC 2012 - mfilka@suse.com

Expand Down