Done.

Don't let me keep this up, from my side feel free to merge.

I created issue811 for turning some of the "const shared_ptr<AbstractTask> &" 
into "const AbstractTask &".

The patch already adheres to the outcome of our discussion in msg7340 and 
msg7341. Should I go ahead and merge it or do you want to have another look?

Sounds good. (Based on your description, no time to look at the code.)

Regarding "const shared_ptr<...> &" vs. "shared_ptr<...>", the former is more
efficient and hence preferable.

I think there are 3 examples of case 4) in the patch:

A) cegar::Abstraction: We need a "const shared_ptr<AbstractTask> &" or 
"shared_ptr<AbstractTask>" here to create an instance of AdditiveHeuristic. 
B) operator_counting::ConstraintGenerator::initialize_constraints: We could 
probably pass a "const AbstractTask &" here. However, this would involve changing 
many files, including all pattern generators.
C) ModifiedOperatorCostsTask constructor: all AbstractTask subclasses that need a 
parent task except for ModifiedOperatorCostsTask currently expect a "const 
shared_ptr<AbstractTask> &". In principle, we could pass "const AbstractTask &" 
instead, but again this would involve changing lots of code.

I think we could open an issue (maybe a meta issue) for turning some of the 
"const shared_ptr<AbstractTask> &" or "shared_ptr<AbstractTask>" into "const 
AbstractTask &", but I'd rather not do it in this issue. If you agree, this 
leaves us with deciding for this issue whether we want to use "const 
shared_ptr<AbstractTask> &" or "shared_ptr<AbstractTask>" for A, B and C. I am in 
favor of using "const shared_ptr<AbstractTask> &" for consistency, but am open to 
any other suggestion.

The changes that would not change the semantics and performance would be these:

1) const bool => bool
2) const vector<...> => vector
3) const AbstractStates => AbstractStates
4) const shared_ptr<...> => shared_ptr

(There may be other kinds of changes that I don't recall.)

You made the type #1 changes, and these are uncontroversial and could be merged
directly.

For #2-#4 you decided to make a semantic change instead and pass by reference. I
presume this is because you think that, e.g., passing a vector in #2 is
obviously not what we want.

I agree that the type #2-#4 examples are not what we want, but I think that if
we think what we currently do is wrong, we need to think about what is the right
thing. For #2 and #3, passing by reference looks good in the examples I've seen.
For #4, it really should be a case-by-case decision.

Point taken. How do we go from here?

Hi Jendrik, but the changes you made are *not* small changes. You replace
pass-by-value by pass-by-reference in many places, which can obviously affect
both the performance and the correctness of the code significantly.

That's true and we might want to change this eventually. However, I tried to keep 
this patch as small as possible. Except for one argument renaming, all changes 
are only there to allow running our current set of clang-tidy checks for header 
files. (I changed the title to reflect this better.)

I checked a few callees, and at least some of them don't seem to require a
shared_ptr, they only used the AbstractTask to create a TaskProxy (which does
not require a shared_ptr, only a const AbstractTask &).

I think we pass "const shared_ptr<X> &" if the callee needs a shared_ptr, e.g., 
for creating a TaskProxy or if the callee wants to store the shared_ptr.

s/contacts/contexts/

I am not sure how descriptive the name is. Changing shared pointers to shared
pointer references is a behavioural change that can in many contacts lead to
crashes due to unaccounted-for changes. The changes may be correct, though I'm
not really sure what we mean semantically by "const shared_ptr<X> &". If we pass
the shared pointer by const reference, it seems we don't participate in the
ownership, so should we not pass a "const X &" instead?

I'm happy about the "const bool" => "bool" changes.

I changed the title to reflect the change of scope of this issue.

I reverted the changes to misc/style/run-all-style-checks.py. 

Regarding system_windows.h, I added two comments on Bitbucket to explain the 
changes. Basically, the code in system_windows.h can't be executed on a Linux 
system. In the other OS-specific files (system_windows.cc and system_unix.cc) 
we use the OPERATING_SYSTEM macro to only run the code on the respective 
platform. I thought we could do the same in system_windows.h.

If you make the changes to the code first, then add the tests to the buildbot
afterwards, things should work nicely. (I wouldn't mind doing them at the same
time if the changes were simpler, but the reasoning behind some of them, such as
the changes to the includes etc. in system_windows.h, is not so clear to me.)

Even if this means that the buildbot tests will fail? I only made the changes 
required for making all tests pass.

Please do the code cleanup separately.

I think it's easiest to just compute the list of source files ourselves, i.e., 
without querying the build system.

I made a pull request at 
https://bitbucket.org/jendrikseipp/downward/pull-requests/90 .

CMake looks for .h and .cc files explicitly:

http://hg.fast-downward.org/file/f1ad0b16caac/src/cmake_modules/FastDownwardMacros.cmake#l213

CMake knows the list of header files internally, otherwise it couldn't know what
to rebuild when a header file changes (or that a rebuild is necessary in the
first place). If and how this information can be accessed is another question.

Since we currently se CMake for reporting the list of source files to clang-tidy 
and CMake knows nothing about header files, these are not checked. I think the 
simplest solution is to create the list of source files ourselves.