Issue607

Title Move planning-independent utility classes to subdirectory
Priority wish Status resolved
Superseder Nosy List florian, jendrik, malte
Assigned To florian Keywords
Optional summary

Created on 2015-12-08.23:26:12 by florian, last changed by florian.

Messages
msg4960 (view) Author: florian Date: 2015-12-11.13:57:05
Merged and pushed as discussed offline.
msg4959 (view) Author: florian Date: 2015-12-11.12:48:45
If you are ok with the changes, Malte, I think this is ready to merge now.
msg4957 (view) Author: jendrik Date: 2015-12-11.11:53:58
I'm done with my review.
msg4956 (view) Author: malte Date: 2015-12-11.11:44:35
> Regarding the automatic merge, I hope that conflicts in the includes can be
> fixed easy enough (usually, take the includes to "utils/*" from default and the
> rest from your branch).

Sure, fixing that conflict itself is no problem, but once you have a conflict in
the file, it's a bit painful to manually merge all the other changes in the
file. I know meld has things like "Merge All Non-Conflicting", but I don't fully
understand yet how they really work. I guess I just need to get to know my tools
better. :-)

> Would you prefer "using namespace Utils;"?

My preferences are not strong enough to override yours. We could ask the others
for their opinions, but we can also go with your preference.

> We could also move make_unique_ptr out of the Utils namespace. I don't know if
> its possible to say "using Utils::make_unique_ptr;", but if it is, that would be
> an option, too.

I'd rather not have one of the functions treated differently from all the others.
msg4953 (view) Author: florian Date: 2015-12-11.09:53:46
Regarding the automatic merge, I hope that conflicts in the includes can be
fixed easy enough (usually, take the includes to "utils/*" from default and the
rest from your branch). For the remaining changes, like in_bounds -->
Utils::in_bounds, I hoped that the merge tool would fix these for you. I
remember kdiff3 merges all non-conflicting changes by default. In meld I saw a
button in the menu labelled "Merge all", but there seems to be a bug in my
version: https://bugs.launchpad.net/ubuntu/+source/meld/+bug/1511945

Anyway, if anyone has problems merging their changes with this issue, I'm happy
to help.
msg4952 (view) Author: florian Date: 2015-12-11.09:26:50
Would you prefer "using namespace Utils;"? We could also move make_unique_ptr
out of the Utils namespace. I don't know if its possible to say "using
Utils::make_unique_ptr;", but if it is, that would be an option, too.
msg4951 (view) Author: malte Date: 2015-12-10.23:49:34
Can't say I'm a fan of having to write "Utils::make_unique_ptr" instead of "new"
all the time in the future, but oh well.

Regarding merge conflicts and automatic merging, I'm skeptical. In my
experience, whenever there are massive changes to the #includes, they trigger
lots of merge conflicts because they are in areas where other changes also cause
errors. (But that's not a reason not to work on this.)
msg4950 (view) Author: jendrik Date: 2015-12-10.20:19:18
Yes, I can :)
msg4949 (view) Author: florian Date: 2015-12-10.20:07:56
I prepared a pull request. This will change a lot of files, but most changes are
small and should be easy to merge automatically.

https://bitbucket.org/flogo/downward-issues/pull-requests/10

A difference to what I wrote earlier is how the new namespace is used. I
discussed this with Jendrik and we decided to use "Utils::foo" instead of "using
namespace Utils;" with one exception: for ExitCodes, we do

  using Utils::ExitCode;
  // ...
  Utils::exit_with(ExitCode::UNSUPPORTED);



Jendrik, can you do a review?
msg4938 (view) Author: malte Date: 2015-12-10.12:11:33
The tracer is really about logging and can be merged with it, I think, as well
as with Jendrik's logging module.

I would combine "system" and "exit_codes" to "system".
msg4937 (view) Author: florian Date: 2015-12-10.11:58:26
Ok, here is an updated suggestion including your comments:

utils/hash:
  hash_combine
  hash_sequence
  hash<vector<T>>
  hash<pair<TA,TB>>

utils/system:
  macro ABORT
  get_peak_memory_in_kb
  get_exit_code_message_reentrant
  is_exit_code_error_reentrant
  register_event_handlers
  report_exit_code_reentrant;
  get_process_id;

utils/exit_codes:
  enum ExitCode 
  exit_with

utils/collections:
  is_sorted_unique
  in_bounds
  swap_and_pop_from_vector
  release_vector_memory

utils/logging
  operator<< for vector<T>

utils/math:
  is_product_within_limit

utils/language:
  macro NO_RETURN
  unused_parameter

utils/memory:
  make_unique_ptr

utils/tracer (maybe combine with utils/memory?):
  TraceBlock
  trace

utils/timer:
  Timer

utils/countdown_timer:
  CountdownTimer

utils/rng:
  RandomNumberGenerator
msg4935 (view) Author: malte Date: 2015-12-10.11:38:44
I think it's insufficient to consider splitting up utilities.{h,cc} in
isolation. I think we should include the other files that we want to move to
utils/ in the discussion. For example, half the functions in system.h are
related to exit codes, so it would be odd to have three functions related to
exit_codes in utils/system.h and then one more (plus one enum) in utils/exit_codes.h


On the proposed divisions:
- I don't think ABORT fits into a header file called "exit_codes", but it could
fit "utils/system". At least in C and Python, program-exit-related functionality
is in "system"-style libraries. (In Python, some are in "os", but there's quite
a bit of overlap between "os" and "sys".)

I'm not a fan of "misc". :-) "math" could work for the product function.
Not so sure about "unused". Perhaps "language" as this is more or less about a
language facility.
For what it's worth, I think "NO_RETURN" should also be in such a language
header rather than "system", where it currently is. We currently associate
NO_RETURN with Windows vs. Linux, but it's really about C++ compilers, not OSes.

The convenience method for operator<< is something I'd associate more with
output/logging etc. than collections, especially if we're going to merge
Jendrik's logging-related functionality. I think it could be useful to have
these output-related things in one place because I think it's more likely that
we want to change operator<< for vectors at times when we reconsider our logging
facilities than at times when we consider modifying things like pop_and_swap or
in_bounds. (One guideline is: code that changes together should live together.)
msg4932 (view) Author: jendrik Date: 2015-12-10.10:40:31
I like your proposal introducing the namespace "Utils" and splitting up the 
files. Maybe the two uncategorized items can go into utils/misc.h.
msg4930 (view) Author: florian Date: 2015-12-09.22:14:17
I checked what we would have to change if we would introduce the namespace
"Utils" for most of this code. Some of the code has to be in the namespace std
anyway (like hash functions), but I put everything else into Util.

As a result I had to put "using namespace Utils;" into roughly 50 *.cc-files.
For most of them it might be nicer to use the namespace only locally, i.e.,
using "Utils::is_sorted_unique" and "Utils::Timer" instead of "using namespace
Utils;". There is only a handful of header files that would be affected.




I also looked at what is in utils/utilities to maybe split this up into separate
classes. I have an initial suggestion as a basis for discussion:

utils/exit_codes:
macro ABORT
enum ExitCode 
exit_with

utils/collections:
is_sorted_unique
in_bounds
swap_and_pop_from_vector
release_vector_memory
operator<< for vector<T>

utils/memory:
make_unique_ptr

I can't really think of the good fit for the remaining two:
unused_parameter (not used currently)
is_product_within_limit


What do you think, should we introduce the namespace, and should we split up
utils/utilities?
msg4928 (view) Author: florian Date: 2015-12-09.17:54:01
Added some TODOs
msg4908 (view) Author: malte Date: 2015-12-09.00:41:26
I'd rather not mix algorithms and data structures stuff like
equivalence_relation.{h.cc}, priority_queue.{h,cc}, int_packer.{h,cc} and
segmented_vector.{h,cc} with system utilities.

Most others sound like would fit into a "utilities" or "utils" directory. (I
think the plural works better if it's a collection of mostly unrelated modules;
cf. plural vs. singular with our current subdirectories.)
msg4906 (view) Author: florian Date: 2015-12-08.23:26:12
I suggest we move most of the utility classes that are not planning specific
into a common subdirectory "utility". I had a look at what they include ("A -->
B" means A includes B):

rng.cc --> utilities.h (actually just needs system.h) for get_process_id
timer.h --> utilities.h (actually just needs system.h) for system detection
countdown_timer.h --> timer.h for base class

system.h --> system_{unix,windows}.h for system specific implementations
system.cc --> utlities.h for exit codes
system_{unix,windows}.cc --> utlities.h for exit codes

utilities.h --> system.h for system specific functions
utilities_hash.cc


I'm not quite sure if the following should be part of the subdirectory:

equivalence_relation.cc --> utilities.h (I don't see why)
priority_queue.cc --> utilities.h (I don't see why)

tracer.cc --> globals.h for g_timer
tracer.cc --> timer.h for g_timer
tracer.cc --> utilities.h (actually just needs system.h) for get_peak_memory_in_kb


I'm even less sure about those two because they are related to the packed
state/state registry implementation:

int_packer.cc
segmented_vector.cc --> utilities.h (I don't see why)
History
Date User Action Args
2015-12-11 13:57:05floriansetstatus: reviewing -> resolved
messages: + msg4960
2015-12-11 12:48:45floriansetmessages: + msg4959
2015-12-11 11:53:58jendriksetmessages: + msg4957
2015-12-11 11:44:35maltesetmessages: + msg4956
2015-12-11 09:53:46floriansetmessages: + msg4953
2015-12-11 09:26:50floriansetmessages: + msg4952
2015-12-10 23:49:34maltesetmessages: + msg4951
2015-12-10 20:19:19jendriksetmessages: + msg4950
2015-12-10 20:07:56floriansetstatus: chatting -> reviewing
messages: + msg4949
summary: TODO: - introduce namespace - split utils/utilities into separate files? - rename utils/utilities? ->
2015-12-10 12:11:33maltesetmessages: + msg4938
2015-12-10 11:58:26floriansetmessages: + msg4937
2015-12-10 11:38:44maltesetmessages: + msg4935
2015-12-10 10:40:31jendriksetmessages: + msg4932
2015-12-09 22:14:17floriansetmessages: + msg4930
summary: TODO: - rename utils/utilities_hash to utils/hash - move g_timer into utils/timer to avoid dependency on globals - introduce namespace - split utils/utilities into separate files? - rename utils/utilities? -> TODO: - introduce namespace - split utils/utilities into separate files? - rename utils/utilities?
2015-12-09 17:54:01floriansetmessages: + msg4928
summary: TODO: - rename utils/utilities_hash to utils/hash - move g_timer into utils/timer to avoid dependency on globals - introduce namespace - split utils/utilities into separate files? - rename utils/utilities?
2015-12-09 00:41:26maltesetmessages: + msg4908
2015-12-08 23:26:12floriancreate