Created on 2015-12-08.23:26:12 by florian, last changed by florian.
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)
|
|
Date |
User |
Action |
Args |
2015-12-11 13:57:05 | florian | set | status: reviewing -> resolved messages:
+ msg4960 |
2015-12-11 12:48:45 | florian | set | messages:
+ msg4959 |
2015-12-11 11:53:58 | jendrik | set | messages:
+ msg4957 |
2015-12-11 11:44:35 | malte | set | messages:
+ msg4956 |
2015-12-11 09:53:46 | florian | set | messages:
+ msg4953 |
2015-12-11 09:26:50 | florian | set | messages:
+ msg4952 |
2015-12-10 23:49:34 | malte | set | messages:
+ msg4951 |
2015-12-10 20:19:19 | jendrik | set | messages:
+ msg4950 |
2015-12-10 20:07:56 | florian | set | status: chatting -> reviewing messages:
+ msg4949 summary: TODO:
- introduce namespace
- split utils/utilities into separate files?
- rename utils/utilities? -> |
2015-12-10 12:11:33 | malte | set | messages:
+ msg4938 |
2015-12-10 11:58:26 | florian | set | messages:
+ msg4937 |
2015-12-10 11:38:44 | malte | set | messages:
+ msg4935 |
2015-12-10 10:40:31 | jendrik | set | messages:
+ msg4932 |
2015-12-09 22:14:17 | florian | set | messages:
+ 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:01 | florian | set | messages:
+ 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:26 | malte | set | messages:
+ msg4908 |
2015-12-08 23:26:12 | florian | create | |
|