The first patch is irrelevant now because the functions like _get_load_path_ no longer accept arguments.

For the second patch, I generally dislike returning pointers or references to internal data members, even if they are constant because it means the programmer needs to ensure that the lifetime of the containing object is longer than the reference to the internal data, and that there are no changes to the containing object that will invalidate the reference or change the value that it refers to unexpectedly.  It seems to me that it is easy to miss details like this when refactoring code in the future, and that can lead to hard to find bugs.  To me, the possible maintenance issues outweigh the performance gains.  Maybe there is another way to do this job that doesn't have the possible maintenance issues?

In the third patch, I agree that setting retval = "" (the first part of the diff) is unnecessary, but setting dir_name = "" in the other two bits of the diff ensures that dir_name is empty if no valid function is found.  That seems reasonable to me, as the user of the function might have passed something else and setting it to "" could avoid later confusion.  I understand these are "internal" functions and that maybe fcn_info "is not supposed to be used that way" but I tend to worry about future maintenance.

[comment #4:]

> The first change seems OK, at least in functions that are called many many times, but I'm not interested in changing all Octave code to use static const std::string objects for every string literal.  The context matters.  For example, there is no need for it in something like
>

> if (some_error_condition)
>   error ("...");

Ye that would be silly. The _get* function signatures could instead be changed to 'const * char' to avoid the const static std::string, since they are just used for a
'.c_str ()' in the end anyway.

> I don't think the other two changes are needed.  I'd rather avoid adding things like "m_empty_string" member variables or const reference for ordinary local variables.  Shouldn't std::move be used in those instances anyway, or does something prevent that from happening?
>

I think std::move is used (the move constructor) but I think there need to be a malloc for the returned string anyways somewhere to have a copy. The shenanigans
with the "m_empty_string" is to allow a const reference for the "return m_rep ? m_rep->m_foo : m_empty_str". A copy of a empty string should not result
in any mallocs due to "short string optimizations", but a copy of m_foo might trigger one depending on size.

I tried patch 2 in isolation and verified that it removes two mallocs per for loop iteration. One because of changes in symscope and one because of changes in fcn-info.

Removing const ref locals in fcn-info.cc with fcn-info.hh as in patch 2 did not remove the malloc.

I have split up the patch accordingly:

Patch 1 - static strings for some _get* functions in fcn-info.cc

Patch 2 - Some const string references instead of copies

Patch 3 - Remove some redundant ""-writes

I ditched the query/process code since it got really kludgy trying to bubble up the result
and it didn't do too much good.

Also I ditched the unordered_map:s since the speed improvement seemed dependent on other patches.

Baseline:

function i = foo ()
  tic;
  for i = 1:100000
    i = 1+2+3+4+5+6+sin(7);
  end
  toc;
end

  37 mallocs per loop iteration

Patch 1:
  Runs about 5% faster than baseline.
  -9 mallocs per loop iteration

Patch 2:
  Unmeasurable speed improvement. 3% according to callgrind.
  -2 mallocs per iteration

Patch 3:
  Unmeasurable speed improvement but verified removes some writes. 0.6% according to callgrind

In total:
  About 5% faster measured. Depends alot on exactly what code is executed. 13% fewer cpu cycles
  according to callgrind.



_run_test_suite_ runs fine* on Debian x64.

  PASS                            16970
  FAIL                                0
  XFAIL (reported bug)               31
  SKIP (missing feature)            155
  SKIP (run-time condition)          26

*(I had to disable eigs' tests since those segfaults on my machine, with or without these patches).

(file #52529, file #52530, file #52531)

This patch is quite large which makes it difficult to review.

Could you please split it up in smaller changes. E.g., one that only changes some `map` objects to `unordered_map`s, another one that replaces string literals with `static const string`s, yet another one that makes `m_fullname` a new class member, ...
That would probably make it much easier to digest.

Additionally, we try to avoid tabulators for white space in code. They display differently for different editors or settings. I've seen pretty much everything from 2 characters to 16 characters per tab... (Is it 8 for your editor?)

I didn't try that patch yet.
Does it compile for you? It looks like the fcn_info_rep constructor is missing an argument. Or I can't see where `m_file_name` gets actually set...